transformations: (pdl) Different type matching between polymorphic and fixed case #3405
Conversation
qaco
added
transformations
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3405 +/- ##
==========================================
+ Coverage 90.16% 90.18% +0.01%
==========================================
Files 456 456
Lines 57540 57567 +27
Branches 5540 5541 +1
==========================================
+ Hits 51883 51917 +34
+ Misses 4197 4191 -6
+ Partials 1460 1459 -1 ☔ View full report in Codecov by Sentry. |
Ok done ! |
| def test_distinct_types_polymorph_matching(): | ||
| @ModuleOp | ||
| @Builder.implicit_region | ||
| def input_module(): | ||
| test.TestOp.create(result_types=[IntegerType(32)]) | ||
| test.TestOp.create(result_types=[IntegerType(64)]) | ||
|
|
||
| @ModuleOp | ||
| @Builder.implicit_region | ||
| def pdl_module(): | ||
| with ImplicitBuilder(pdl.PatternOp(2, None).body): | ||
| pdl_type = pdl.TypeOp().result | ||
| pdl_op = pdl.OperationOp(StringAttr("test.op"), type_values=[pdl_type]).op | ||
| with ImplicitBuilder(pdl.RewriteOp(pdl_op).body): | ||
| pdl.EraseOp(pdl_op) | ||
|
|
||
| pdl_rewrite_op = next( | ||
| op for op in pdl_module.walk() if isinstance(op, pdl.RewriteOp) | ||
| ) | ||
|
|
||
| ctx = MLContext() | ||
| pattern_walker = PatternRewriteWalker(PDLRewritePattern(pdl_rewrite_op, ctx)) | ||
|
|
||
| pattern_walker.rewrite_module(input_module) | ||
| assert input_module.is_structurally_equivalent(ModuleOp([])) | ||
|
|
||
|
|
||
| def test_distinct_types_matching(): | ||
| @ModuleOp | ||
| @Builder.implicit_region | ||
| def input_module(): | ||
| test.TestOp.create(result_types=[IntegerType(32)]) | ||
| test.TestOp.create(result_types=[IntegerType(64)]) | ||
|
|
||
| @ModuleOp | ||
| @Builder.implicit_region | ||
| def output_module(): | ||
| test.TestOp.create(result_types=[IntegerType(64)]) | ||
|
|
||
| @ModuleOp | ||
| @Builder.implicit_region | ||
| def pdl_module(): | ||
| with ImplicitBuilder(pdl.PatternOp(2, None).body): | ||
| pdl_type = pdl.TypeOp(IntegerType(32)).result | ||
| pdl_op = pdl.OperationOp(StringAttr("test.op"), type_values=[pdl_type]).op | ||
| with ImplicitBuilder(pdl.RewriteOp(pdl_op).body): | ||
| pdl.EraseOp(pdl_op) | ||
|
|
||
| pdl_rewrite_op = next( | ||
| op for op in pdl_module.walk() if isinstance(op, pdl.RewriteOp) | ||
| ) | ||
|
|
There was a problem hiding this comment.
I apologise, I should have been more precise! I didn't mean for you to do all this work to reimplement the inputs etc. In my head, it was a much smaller function, and I didn't realise that there was no precedent already in this file for it. Here is what I was hoping we would already have in this file:
def test_match_type():
matcher = PDLMatcher()
pdl_op = pdl.TypeOp()
ssa_value = pdl_op.result
xdsl_value = StringAttr("a")
# New value
assert matcher.match_type(ssa_value, pdl_op, xdsl_value)
assert matcher.matching_context == {ssa_value: xdsl_value}
# Same value
assert matcher.match_type(ssa_value, pdl_op, xdsl_value)
assert matcher.matching_context == {ssa_value: xdsl_value}
# Other value
assert not matcher.match_type(ssa_value, pdl_op, StringAttr("b"))
assert matcher.matching_context == {ssa_value: xdsl_value}I was thinking that it would be just 5 minutes work to add the new test cases for the functionality you added. As it stands these tests are duplicating the functionality tested in the filecheck test, which is probably the better place for them now that you've added the filecheck functionality.
I don't want to block you any more, so if you would like to merge as-is, I think that would also be ok, but if you have the time to replace these two tests with a more targeted test like the one above that just checks the one function you modified I think it would help debug in the future.
There was a problem hiding this comment.
Oh, ok, I didn't get what you requested. That's fine, I'll do it !
Dos it mean that I do the merge ? |
|
Yep, all you need is one green tick, and no leftover comments from the review |
…d fixed case (xdslproject#3405) Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
No description provided.