Core: Add NoTerminator trait #1049

compor · 2023-06-01T10:00:40Z

This PR adds the NoTerminator trait.

A current issue is that the IRDL annotations (e.g., SingleBlockRegion, etc) operate separately from this trait.
I'm not sure if the link between the two should be checked before an operation's verify method, i.e. during construction, or type-checked in this trait's verify method.

Any feedback on this is welcome.

codecov · 2023-06-01T10:05:24Z

Codecov Report

Patch coverage: 99.09% and project coverage change: +0.02 🎉

Comparison is base (f86a6ea) 88.66% compared to head (22879d7) 88.68%.

❗ Current head 22879d7 differs from pull request most recent head 48ef91a. Consider uploading reports for the commit 48ef91a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1049      +/-   ##
==========================================
+ Coverage   88.66%   88.68%   +0.02%     
==========================================
  Files         160      160              
  Lines       22300    22332      +32     
  Branches     3345     3356      +11     
==========================================
+ Hits        19773    19806      +33     
+ Misses       1989     1988       -1     
  Partials      538      538

Impacted Files	Coverage Δ
docs/Toy/toy/dialects/toy.py	`68.12% <ø> (ø)`
tests/dialects/test_affine.py	`100.00% <ø> (ø)`
xdsl/ir/core.py	`88.37% <92.30%> (+<0.01%)`	⬆️
tests/dialects/test_func.py	`100.00% <100.00%> (ø)`
tests/dialects/test_scf.py	`100.00% <100.00%> (ø)`
tests/test_builtin_traits.py	`100.00% <100.00%> (ø)`
tests/test_ir.py	`100.00% <100.00%> (ø)`
tests/test_operation_builder.py	`100.00% <100.00%> (ø)`
xdsl/dialects/builtin.py	`83.84% <100.00%> (ø)`
xdsl/dialects/gpu.py	`98.76% <100.00%> (ø)`
... and 5 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

math-fehr

I need to check in the MLIR codebase, but what we should have is somewhere a check that all blocks have terminator. I need to look where this check is done though!

tests/test_builtin_traits.py

math-fehr · 2023-06-01T13:37:37Z

xdsl/traits.py

+
+    def verify(self, op: Operation) -> None:
+        for r in op.regions:
+            if len(r.blocks) > 1:


It is actually a bit more complex than that. Here is the "documentation" I could find in MLIR:

In our case, we don't actually need to verify anything in the trait. What we need is somewhere a check that blocks have terminator if the conditions are respected. I'm just not sure where to put it yet, I'll take a look now.

yeah, I found that as well yesterday, good to see, but when I tried to check for the NoTerminator trait in ir.py (simulating this), I got a circular dependency in my imports, so that kinda put me off from going that route.

~~Moreover, I couldn't find any validation for terminators in xDSL.~~ (see comment below)

math-fehr · 2023-06-01T13:42:53Z

Looking at the MLIR codebase, the check that blocks have terminators should be done in the verify function in Operation between these two code blocks:

compor · 2023-06-01T13:52:24Z

Looking at the MLIR codebase, the check that blocks have terminators should be done in the verify function in Operation between these two code blocks:

I see, and just to be clear about my understanding, terminators are operations with a non-empty successor list in xDSL, correct?

superlopuh · 2023-06-01T21:58:31Z

If I understand correctly, while this trait would be useful for xDSL in general, we'd still want terminators in pretty much all RISC-V blocks, right? @math-fehr

superlopuh · 2023-06-01T21:59:05Z

At least the ones that represent code as opposed to data sections

compor · 2023-06-01T22:13:30Z

At least the ones that represent code as opposed to data sections

That is my understanding by looking at the MLIR verifier.
Once this is in, we could annotate RISCV blocks without jump operations with this trait by default.
Not sure how prudent that is.

Otherwise, maybe we could have a TerminatorOp that doesn't emit assembly.

…not last in it

superlopuh · 2023-06-02T05:38:34Z

I think that's standard, something like a yield op that just carries on to its successor

math-fehr · 2023-06-02T10:53:42Z

Terminator is probably the trait you want to implement first actually!
It should be a trait on operations. And operations should not have any successors unless they have the Terminator trait (this is something that I can add myself on IRDL later on).
Any region that has multiple blocks should have terminators on these blocks, so not sure what you have in your Riscv blocks.

I guess the steps would be:

Adding the Terminator trait, and the check in Operation.verify that blocks have terminators, besides the single block ones (so it is easier to write the next patch)
Add the NoTerminator trait, and ensure all single block regions have terminators unless they have this trait.

compor · 2023-06-02T12:03:19Z

Terminator is probably the trait you want to implement first actually! It should be a trait on operations. And operations should not have any successors unless they have the Terminator trait (this is something that I can add myself on IRDL later on). Any region that has multiple blocks should have terminators on these blocks, so not sure what you have in your Riscv blocks.

I guess the steps would be:
* Adding the `Terminator` trait, and the check in `Operation.verify` that blocks have terminators, besides the single block ones (so it is easier to write the next patch)

* Add the `NoTerminator` trait, and ensure all single block regions have terminators unless they have this trait.

Yeah, I was going to open a PR later for this, but I guess it makes sense to add it first.

Regarding, the MLIR implementation of this and mayBeValidWithoutTerminator utility method, it seems it applies the aforementioned "rules" twice, once for empty blocks and non-empty ones, but only after the successors go through sanity checks and that is within verifyBlock, and that seems to work for us as well.
I'm just saying this because implementing it within operation felt a bit awkward, but I can come back to it.

superlopuh · 2023-06-02T12:08:35Z

Seems like riscv.ret would benefit from the Terminator trait, and then we could add a yield operation for other control flow like things

compor · 2023-06-02T12:13:17Z

Seems like riscv.ret would benefit from the Terminator trait, and then we could add a yield operation for other control flow like things

yeah exactly and combine that with NoTerminator in other cases (considering the initial discussion that sparked this)

review-notebook-app · 2023-06-19T07:40:37Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

math-fehr

A big PR, but I don't think we can reduce it much further!

math-fehr · 2023-06-20T13:20:15Z

tests/filecheck/parser-printer/unregistered_dialect.mlir

@@ -6,12 +6,14 @@
    %y = "op_with_res"() {otherattr = #unknown_attr<b2 ...2 [] <<>>>} : () -> (i32)
    %z = "op_with_operands"(%y, %y) : (i32, i32) -> !unknown_type<{[<()>]}>
    "op"() {ab = !unknown_singleton_type} : () -> ()
+    "test.termop"() : () -> ()


That looks suspicious to me.
If an operation is not registered, we cannot require a terminator, as we don't know if the operation has a NoTerminator trait or not.

Actually, let's do that in another PR, I need to add HasMaybeTrait that does exactly that.

hmm, that goes back to a question I had regarding unregistered ops in xDSL.
I can have a look at that, basically, it will be modelled after hasTrait but take into account if an op is not registered, right?

I opened an issue to track this #1167

xdsl/dialects/stencil.py

xdsl/ir/core.py

math-fehr · 2023-06-20T13:38:49Z

xdsl/ir/core.py

-                    )
+            if parent_block.last_op == self:
+                if len(parent_region.blocks) == 1:
+                    if (


Is there a case missing where we have NoTerminator but have been given a terminator?
Or is this handle somewhere else?

do you mean the check in Block.verify()?

I don't think this checks it as well.
If we have

builtin.module { test.term_op }

this should fail because the last operation of a NoTerminator operation is a terminator.
But I don't see any check for that here or in Block.verify()?

Hmm, forget my Block.verify comment, I misunderstood there.
The way I read it was that if the NoTerminator trait is applied, no need to do any further checking.
If it doesn't exist, then the IsTerminator is required on the last operation.

Basically, the last few lines here.

I've added a test case regarding this @math-fehr

webmiche

LGTM, thanks for the PR :)

math-fehr

👍

compor added 3 commits June 1, 2023 10:39

core: Add NoTerminator trait

ce4c909

core: Add NoTerminator trait tests

b3d9901

Update docstring

7fb8b7a

compor added the core xDSL core (ir, textual format, ...) label Jun 1, 2023

compor self-assigned this Jun 1, 2023

compor requested review from math-fehr, webmiche and georgebisbas as code owners June 1, 2023 10:00

math-fehr reviewed Jun 1, 2023

View reviewed changes

compor added 2 commits June 1, 2023 16:53

Use pytest.raise()'s regex for exception message matching

8ecff35

Remove verification from trait

81e7b18

compor added 2 commits June 1, 2023 23:36

Add check for successors in non-empty blocks

84c94bf

Add test case for block containing operation with successors that is …

a660ca7

…not last in it

compor added 3 commits June 2, 2023 11:42

Use correct operation that allows successors

e60e4bc

Add branching to another region check for blocks

2529270

Add test for branching to another region check for blocks

71ec8dc

compor added 3 commits June 3, 2023 14:55

Add tests checking block terminator conditions

3cc9319

Add utility method checking block terminator conditions

dfbdd51

Use utility method for terminator conditions

ab71e69

This was referenced Jun 19, 2023

dialects/core: Add missing IsTerminator and HasParent traits to dialects #1154

Merged

core: Simplify successor terminator conditions #1156

Merged

compor added 8 commits June 19, 2023 14:17

Merge branch 'main' into christos/core/no-term-trait

da5130e

Remove redundant definition of traits

9481dcd

Remove redundant test case handled by traits

3824e69

Add test case

b9488ac

Merge branch 'main' into christos/core/no-term-trait

3f889b2

Remove tests handled by traits

3baf6f5

Add more tests

db91a03

Fix scf test coverage

66b146f

compor changed the title ~~[WIP] Core: Add NoTerminator trait~~ Core: Add NoTerminator trait Jun 20, 2023

compor requested review from math-fehr and superlopuh June 20, 2023 13:16

math-fehr approved these changes Jun 20, 2023

View reviewed changes

Elide duplicated trait

5e97ae9

superlopuh approved these changes Jun 20, 2023

View reviewed changes

Split up long strings

bbfa818

webmiche approved these changes Jun 20, 2023

View reviewed changes

Add test case

b917459

compor mentioned this pull request Jun 21, 2023

Fix terminator conditions for unregistered operations #1167

Closed

Merge branch 'main' into christos/core/no-term-trait

5f8d41a

compor requested a review from math-fehr June 21, 2023 08:14

compor added 3 commits June 21, 2023 10:55

Merge branch 'main' into christos/core/no-term-trait

6fc2d21

Merge branch 'main' into christos/core/no-term-trait

2963682

Remove deprecate func.Return instantiation

22879d7

math-fehr approved these changes Jun 21, 2023

View reviewed changes

Merge branch 'main' into christos/core/no-term-trait

48ef91a

compor merged commit d8e60cd into main Jun 21, 2023

compor deleted the christos/core/no-term-trait branch June 21, 2023 13:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Add NoTerminator trait #1049

Core: Add NoTerminator trait #1049

compor commented Jun 1, 2023

codecov bot commented Jun 1, 2023 •

edited

Loading

math-fehr left a comment

math-fehr Jun 1, 2023

compor Jun 1, 2023 •

edited

Loading

math-fehr commented Jun 1, 2023

compor commented Jun 1, 2023 •

edited

Loading

superlopuh commented Jun 1, 2023

superlopuh commented Jun 1, 2023

compor commented Jun 1, 2023

superlopuh commented Jun 2, 2023

math-fehr commented Jun 2, 2023

compor commented Jun 2, 2023

superlopuh commented Jun 2, 2023

compor commented Jun 2, 2023

review-notebook-app bot commented Jun 19, 2023

math-fehr left a comment

math-fehr Jun 20, 2023

math-fehr Jun 20, 2023

compor Jun 20, 2023 •

edited

Loading

compor Jun 21, 2023

math-fehr Jun 20, 2023

compor Jun 20, 2023 •

edited

Loading

math-fehr Jun 20, 2023

compor Jun 20, 2023 •

edited

Loading

compor Jun 21, 2023 •

edited

Loading

webmiche left a comment

math-fehr left a comment

Core: Add NoTerminator trait #1049

Core: Add NoTerminator trait #1049

Conversation

compor commented Jun 1, 2023

codecov bot commented Jun 1, 2023 • edited Loading

Codecov Report

math-fehr left a comment

Choose a reason for hiding this comment

math-fehr Jun 1, 2023

Choose a reason for hiding this comment

compor Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

math-fehr commented Jun 1, 2023

compor commented Jun 1, 2023 • edited Loading

superlopuh commented Jun 1, 2023

superlopuh commented Jun 1, 2023

compor commented Jun 1, 2023

superlopuh commented Jun 2, 2023

math-fehr commented Jun 2, 2023

compor commented Jun 2, 2023

superlopuh commented Jun 2, 2023

compor commented Jun 2, 2023

review-notebook-app bot commented Jun 19, 2023

math-fehr left a comment

Choose a reason for hiding this comment

math-fehr Jun 20, 2023

Choose a reason for hiding this comment

math-fehr Jun 20, 2023

Choose a reason for hiding this comment

compor Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

compor Jun 21, 2023

Choose a reason for hiding this comment

math-fehr Jun 20, 2023

Choose a reason for hiding this comment

compor Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

math-fehr Jun 20, 2023

Choose a reason for hiding this comment

compor Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

compor Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

webmiche left a comment

Choose a reason for hiding this comment

math-fehr left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 1, 2023 •

edited

Loading

compor Jun 1, 2023 •

edited

Loading

compor commented Jun 1, 2023 •

edited

Loading

compor Jun 20, 2023 •

edited

Loading

compor Jun 20, 2023 •

edited

Loading

compor Jun 20, 2023 •

edited

Loading

compor Jun 21, 2023 •

edited

Loading