interpreter: (pdl) add an initial version of EqSat PDL Interpreter #3572
Conversation
xdsl/interpreters/eqsat_pdl.py
Outdated
| @dataclass | ||
| class EqsatPDLMatcher(PDLMatcher): | ||
| def match_operand( | ||
| self, ssa_val: SSAValue, pdl_op: pdl.OperandOp, xdsl_val: SSAValue | ||
| ): | ||
| owner = xdsl_val.owner | ||
| assert isinstance(owner, eqsat.EClassOp) | ||
| assert ( | ||
| len(owner.operands) == 1 | ||
| ), "newly converted eqsat always has 1 element in eclass" | ||
| arg = owner.operands[0] | ||
| res = super().match_operand(ssa_val, pdl_op, arg) | ||
| # self.value_to_eclass[arg] = owner | ||
| return res |
There was a problem hiding this comment.
could you please open a first PR that just adds this, and adds a corresponding pytest unit test for this in the style of the existing pdl matching tests?
There was a problem hiding this comment.
Do you mean splitting this PR into two (Matcher and Rewriter)?
There was a problem hiding this comment.
Created an PR here: #3574
There was a problem hiding this comment.
@jianyicheng - is it correct that this assert implies the matched e-class has at most one element?
If I update the testcase to respect (a+b)+c - then you get an e-class with two elements - is that correct?
There was a problem hiding this comment.
@cowardsa
I think this PR only targets the example of swapping inputs for (a+b) to help review. Future PRs can target (a+b)+c progressively.
xdsl/transforms/apply_eqsat_pdl.py
Outdated
| if isinstance(op, pdl.RewriteOp) | ||
| ] | ||
| pattern_applier = GreedyRewritePatternApplier(rewrite_patterns) | ||
| # TODO: remove apply_recursively=False |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3572 +/- ##
========================================
Coverage 90.40% 90.40%
========================================
Files 469 470 +1
Lines 58907 59058 +151
Branches 5605 5625 +20
========================================
+ Hits 53252 53389 +137
- Misses 4213 4221 +8
- Partials 1442 1448 +6 ☔ View full report in Codecov by Sentry. |
|
@superlopuh Do we need to split this PR further, or this is okay to merge? |
| self.interpreter.push_scope("rewrite") | ||
| self.interpreter.set_values(matcher.matching_context.items()) | ||
| self.functions.rewriter = rewriter | ||
| # self.functions.value_to_eclass = matcher.value_to_eclass |
| assert pdl_op_val is not None, "TODO: handle None root op in pdl.RewriteOp" | ||
| assert ( | ||
| self.pdl_rewrite_op.body is not None | ||
| ), "TODO: handle None body op in pdl.RewriteOp" |
There was a problem hiding this comment.
could you please raise DiagnosticExceptions here and add filecheck tests for them?
| # How to deal with operandSegmentSizes? | ||
| # operand_values, attribute_values, type_values = args |
There was a problem hiding this comment.
| # How to deal with operandSegmentSizes? | |
| # operand_values, attribute_values, type_values = args |
|
Ideally we'd like unit tests for each interpreter method, but I also think that it should be ok to add these as this project progresses further. I added some TODOs as I was prototyping things, but we try to avoid them in main, could you please add a test that checks for these specifically? Ideally also for the |
This is an initial attempt of adding a PDL interpreter for EqSat because rewrites in EqSat does not replace operations but append new operations into existing ops.
This PR also includes a unit test for the eqsat interpreter.