Cleanup apply_deltas #1464
Cleanup apply_deltas #1464
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1464 +/- ##
==========================================
- Coverage 77.00% 76.97% -0.03%
==========================================
Files 209 209
Lines 22440 22421 -19
Branches 3807 3802 -5
==========================================
- Hits 17279 17259 -20
+ Misses 4444 4443 -1
- Partials 717 719 +2 ☔ View full report in Codecov by Sentry. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Test Results 28 files ± 0 28 suites ±0 2h 39m 51s ⏱️ - 10m 29s For more details on these failures, see this check. Results for commit 5543eb9. ± Comparison against base commit 997beb2. This pull request removes 4 and adds 2026 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
| @@ -467,7 +462,7 @@ def matches_node(self, node: ir.Node, model: ir.Model) -> MatchResult: | |||
| return MatchResult.FAIL() | |||
| if not self.op.matches(node.op_type): | |||
| return MatchResult.FAIL() | |||
| match = MatchResult([]) | |||
| match = MatchResult(success=True) | |||
There was a problem hiding this comment.
If the usage is to assume it's a success in the begining, should we set a default to MatchResult?
| def values(self) -> Sequence[Any] | None: | ||
| return self.matched_values | ||
| def nodes(self) -> Sequence[ir.Node]: | ||
| return self.matched_nodes |
There was a problem hiding this comment.
Does it mean match_nodes can be private?
|
|
||
| def extend(self, other: MatchResult | bool): | ||
| if not self.success: | ||
| return | ||
| if not other: | ||
| self.fail() |
There was a problem hiding this comment.
Curious how fail() was designed and what are the thoughts for setting a boolean state instead? Just for my learning. Thanks!
There was a problem hiding this comment.
No deep thought here. Not sure about your question, I am not altering the behavior/approach here, merely inlining the definition of fail(). On second thoughts, retaining the method might be useful ... let me think, anyway, there's more to come, since I'd like to merge this with Xavier's MatchResult definition eventually.
| # Propagate relevant info from old value to new value | ||
| # TODO(Rama): Perhaps we should merge old and new types. As of now, new | ||
| # values don't have type information. Note that this could be a problem | ||
| # for semantics-altering rewrite-rules: we should allow users to override | ||
| # this for such rules. | ||
| new_value.type = old_value.type | ||
| new_value.shape = old_value.shape | ||
| new_value.const_value = old_value.const_value | ||
| new_value.name = old_value.name |
There was a problem hiding this comment.
Should we extract this as a helper for now?
Co-authored-by: Ti-Tai Wang <[email protected]>
Co-authored-by: Justin Chu <[email protected]>
Co-authored-by: Justin Chu <[email protected]>
Next step in cleanup of pattern-matcher: * Each rewrite-transformation is applied immediately when it is identified (since the IR now allows us to do this). * Eliminate the extra checks to avoid overlapping matches (because of above) * Use a structured representation of the delta * Enable function constructing the replacement to return None (failure of rewrite) * Cleanup MatchResult * Other naming cleanup TODO: * Consider extending delta representation to capture old/new output-values --------- Co-authored-by: Ti-Tai Wang <[email protected]> Co-authored-by: Justin Chu <[email protected]>
Next step in cleanup of pattern-matcher:
TODO: