transformations: (apply-individual-rewrite) lazily find canonicalization patterns #3631
Conversation
superlopuh
requested review from
alexarice,
math-fehr,
PapyChacal,
qaco,
webmiche and
dshaaban01
| matched_operation_index: int | None = None | ||
| operation_name: str | None = None | ||
| pattern_name: str | None = None |
There was a problem hiding this comment.
none of these were really optional
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3631 +/- ##
==========================================
- Coverage 90.74% 90.73% -0.01%
==========================================
Files 465 465
Lines 58733 58734 +1
Branches 5629 5632 +3
==========================================
- Hits 53296 53292 -4
- Misses 3997 4000 +3
- Partials 1440 1442 +2 ☔ View full report in Codecov by Sentry. |
superlopuh
changed the title
uv.lock
Outdated
| @@ -2390,7 +2390,7 @@ wheels = [ | |||
|
|
|||
| [[package]] | |||
| name = "xdsl" | |||
| version = "0.25.0+89.gb85bb1a8" | |||
| version = "0.25.0+37.g414bcb0e.dirty" | |||
| ] = { | ||
| arith.AddiOp: (AdditionOfSameVariablesToMultiplyByTwo(),), | ||
| arith.DivUIOp: (DivisionOfSameVariableToOne(),), | ||
| INDIVIDUAL_REWRITE_PATTERNS_BY_NAME: dict[str, dict[str, RewritePattern]] = { |
There was a problem hiding this comment.
I don't understand why this change fits into this PR? It's also unclear to me that matching on a string is better than matching on a type
There was a problem hiding this comment.
Further, why change this to not being a tuple?
There was a problem hiding this comment.
On further thought, why is this necessary at all. Can these two just be made into canonicalization patterns?
There was a problem hiding this comment.
I promise to make this much neater in the near future, this is just a stop-gap to not change everything at once
There was a problem hiding this comment.
It's at least not a regression
| pattern_name: str | None = None | ||
| matched_operation_index: int = field() | ||
| operation_name: str = field() | ||
| pattern_name: str = field() |
There was a problem hiding this comment.
I had been thinking that it might be more ergonomic here to just store the pattern directly here, instead of storing the name of the pattern and then looking it up, as we currently duplicate the lookup. Thoughts?
There was a problem hiding this comment.
The design is there to accomodate saving the pass pipeline and then reusing it later, and not have it just as state in memory that can't be reproduced without clicking on the same buttons in the ui
There was a problem hiding this comment.
fair enough, that makes a lot of sense
| associated with each operation, and the values are the corresponding RewritePattern | ||
| instances. | ||
| """ | ||
| def _get_pattern(op: Operation, pattern_name: str) -> RewritePattern | None: |
There was a problem hiding this comment.
| def _get_pattern(op: Operation, pattern_name: str) -> RewritePattern | None: | |
| def _get_canonicalization_pattern(op: Operation, pattern_name: str) -> RewritePattern | None: |
I thought this was including the individual rewrites at first
alexarice
changed the title
It's time to make this part of the codebase a bit more extensible and robust, as we will be reusing it for some scheduling exploration. As a first step, this PR changes the rewrite infrastructure to get the available patterns on the operation lazily, instead of by scanning all ops of all dialects.