transforms: use rewriter and listener in convert-stencil-to-csl-stencil #3538
Conversation
`PatternRewriter` should only be used for rewrite patterns. stack-info: PR: #3537, branch: math-fehr/stack/1
The pass was not propagating the listener from the PatternRewriter, and thus some operations were modified without notifying the rewrite worklist. stack-info: PR: #3538, branch: math-fehr/stack/2
f9c786f to
afc4913
Compare
dae783a to
eb30740
Compare
|
@n-io, can you confirm that the |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3538 +/- ##
==========================================
- Coverage 90.40% 90.40% -0.01%
==========================================
Files 467 467
Lines 58861 58862 +1
Branches 5605 5605
==========================================
Hits 53213 53213
- Misses 4206 4207 +1
Partials 1442 1442 ☔ View full report in Codecov by Sentry. |
|
Yes, the order is irrelevant since the stencil index is stored with the coefficient value. |
| @@ -505,12 +506,13 @@ def match_and_rewrite(self, op: stencil.ApplyOp, rewriter: PatternRewriter, /): | |||
| # add operations from list to receive_chunk, use translation table to rebuild operands | |||
| for o in chunk_region_ops: | |||
| if isinstance(o, stencil.ReturnOp | csl_stencil.YieldOp): | |||
| rewriter.erase_op(o) | |||
There was a problem hiding this comment.
Why is this new erase_op necessary?
There was a problem hiding this comment.
good question, if I had to guess, I'd say it seems like defensive programming to me as the yield is added anyhow below. Unless there's something else @math-fehr ?
There was a problem hiding this comment.
The operation was just unused anywhere, and not deleted.
In particular, that means it was still in the greedy pattern rewriter worklist, and was being matched on.
The last PR in the list expose that error, as now it tries to still match it, but fails because it shouldn't match any operation that has no parents.
| @@ -505,12 +506,13 @@ def match_and_rewrite(self, op: stencil.ApplyOp, rewriter: PatternRewriter, /): | |||
| # add operations from list to receive_chunk, use translation table to rebuild operands | |||
| for o in chunk_region_ops: | |||
| if isinstance(o, stencil.ReturnOp | csl_stencil.YieldOp): | |||
| rewriter.erase_op(o) | |||
There was a problem hiding this comment.
good question, if I had to guess, I'd say it seems like defensive programming to me as the yield is added anyhow below. Unless there's something else @math-fehr ?
…project#3537) Stacked PRs: * xdslproject#3540 * xdslproject#3539 * xdslproject#3538 * __->__#3537 --- --- --- ### transforms: use Rewriter instead of PatternRewriter in mlir-opt `PatternRewriter` should only be used for rewrite patterns.
…il (xdslproject#3538) Stacked PRs: * xdslproject#3540 * xdslproject#3539 * __->__#3538 * xdslproject#3537 --- --- --- ### transforms: use rewriter and listener in convert-stencil-to-csl-stencil The pass was not propagating the listener from the PatternRewriter, and thus some operations were modified without notifying the rewrite worklist.
Stacked PRs:
transforms: use rewriter and listener in convert-stencil-to-csl-stencil
The pass was not propagating the listener from the PatternRewriter, and
thus some operations were modified without notifying the rewrite
worklist.