dialects: (scf) add index_switch #3157
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3157 +/- ##
==========================================
+ Coverage 90.00% 90.03% +0.03%
==========================================
Files 427 430 +3
Lines 53860 54067 +207
Branches 8344 8367 +23
==========================================
+ Hits 48477 48681 +204
- Misses 4036 4038 +2
- Partials 1347 1348 +1 ☔ View full report in Codecov by Sentry. |
xdsl/dialects/scf.py
Outdated
| raise VerifyException("case values should have type i64") | ||
|
|
||
| if len(self.cases.data) != len(self.case_regions): | ||
| raise VerifyException( | ||
| f"has {len(self.case_regions)} case regions but {len(self.cases.data)} case values" | ||
| ) | ||
|
|
||
| cases = self.cases.data.data | ||
| if len(set(cases)) != len(cases): | ||
| raise VerifyException("has duplicate case value") | ||
|
|
||
| def verify_region(region: Region, name: str): | ||
| yield_op = region.block.last_op | ||
| if not yield_op or not isinstance(yield_op, Yield): | ||
| raise VerifyException(f"expected region {name} to end with `scf.yield`") | ||
|
|
||
| if yield_op.operand_types != self.result_types: | ||
| raise VerifyException( | ||
| f"region {name} returns values of types {yield_op.operand_types}" | ||
| f"but expected {self.result_types}" | ||
| ) |
There was a problem hiding this comment.
could you please add tests for these?
xdsl/dialects/scf.py
Outdated
| verify_region(self.default_region, "default") | ||
| for name, region in zip(cases, self.case_regions): | ||
| verify_region(region, str(name)) |
There was a problem hiding this comment.
The verification is a direct translation of the verification for the mlir operation. It's as necessary as any verification I guess
There was a problem hiding this comment.
I expect I could use SingleBlockImplicitTerminator and leave it at that?
There was a problem hiding this comment.
My impression is that the regions of the operation are already verified by the time the custom verifier is called, is that not the case? Can you check that they're not verified twice?
There was a problem hiding this comment.
verify_region is a custom function defined above
There was a problem hiding this comment.
Ah, I see, I thought we had Operation.verify call into helper methods on the operation, and that this was also doing this manually. In that case all good, although maybe it might be better as a private method?
There was a problem hiding this comment.
Sorry, github hid this from me before merging, is the method not private anyway given it's defined locally in the function? (Given this is python I am expecting the answer to be no)
There was a problem hiding this comment.
I should really be a bit more attentive in PRs, I'm not sure how I missed it. It's private indeed, but that just means that you define it every time that you verify this operation. Ideally, it should be a static method with an underscore, so that it's defined only once, when the file is imported.
There was a problem hiding this comment.
No problem, I'll make a quick pr
|
Added the appropriate traits as they should have been there anyway |
Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
Adds
scf.index_switchand test