transformations: (convert-pdl-to-pdl-interp) Add eqsat instrumentation#5585
transformations: (convert-pdl-to-pdl-interp) Add eqsat instrumentation#5585
Conversation
With the foreach approach, I believe part of this logic can be simplified/omitted, but this is a good first step.
We need to be more careful with the bindings selected by matchers. Right now, values are sometimes passed as their class result to a rewriter: ``` x = … y = … c = class x, y rewrite “myrewrite”(c) ``` This is problematic in MLIR as other rewrites can invalidate c. While we are guaranteed to not erase any operation in the “program domain”, this is not true for the operations of the “e-graph domain”. The solution is to ensure in the pdl lowering that only values from the program domain are passed to rewriters and the conversion to e-graph domain happens at the latest possible point, in the rewriter itself. (This is not an issue in xDSL because erasing an e-class does not discard of the object)
non-eqsat lowering should stay the same
jumerckx
force-pushed
the
jumerckx/pdl-to-pdl-interp-eqsat
branch
from
84a0313 to
c8f2a47
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5585 +/- ##
==========================================
- Coverage 86.25% 86.22% -0.03%
==========================================
Files 403 403
Lines 56660 57186 +526
Branches 6512 6641 +129
==========================================
+ Hits 48871 49310 +439
- Misses 6268 6321 +53
- Partials 1521 1555 +34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jumerckx
force-pushed
the
jumerckx/pdl-to-pdl-interp-eqsat
branch
from
15d9028 to
956df7a
Compare
(not entirely done) Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
|
Hi @jumerckx, can you please take a look at the CI failures? seems like it should be good to merge with those resolved |
jumerckx
force-pushed
the
jumerckx/pdl-to-pdl-interp-eqsat
branch
from
795a3ab to
2b9401f
Compare
often it doesn't seem to do anything on top of the sorted list, but better safe than sorry?
|
Done |
|
Actually I just noticed the 200 missing lines in coverage, it seems that there are no tests for the actual conversion? Could you please add some? I'm not sure how best to test this since it's quite large, but it would be good to have at least some way of catching changes in behaviour, even if they're not the platonic ideal of a unit test... |
|
How do ChooseNodes get generated? Should we add a second smoke test with optimize for eqsat being true to show the difference in output? |
|
I added a small smoketest, you want me to cover things better? |
ChooseNodes are not directly visible in the generated output. The are created The smoketest is with |
|
I have a feeling something went wrong with editing the last message, you didn't finish explaining how they are created :) Codecov seems to not pick up them being created with optimizations on or off? |
Choosenodes are inserted at every branch point where a defining operation from a different location is required. First the outer operation ( In the current implementation, having different patterns that require separate defining operations but share the exact same matching until that point is exceedingly rare so ChooseNodes typically have one choice. There is definitely some low-hanging fruit should we want to optimize this lowering further. The test does actually create and generate code from a (single-choice) ChooseNode. |
2 participants
if self.optimize_for_eqsatoptimize_for_eqsat, order constraints such thatget_defining_ops are spread apart.