Core: Add IsTerminator trait #1080

compor · 2023-06-05T15:14:28Z

This PR adds the IsTerminator trait.

It is a spinoff of the discussion here and specifically this recommendation regarding the initial (and pending) attempt to add the NoTerminator trait.

xdsl/ir.py

math-fehr

Nice! Happy to see that coming!

xdsl/ir.py

xdsl/traits.py

math-fehr · 2023-06-05T18:05:12Z

xdsl/ir.py

+                            "Operation with block successors must terminate its parent block"
+                        )
+
+                    if not self.has_trait(IsTerminator):


This (len(self.successors) > 0 -> has_trait(IsTerminator)) should be checked even if the operation has no parents right?
Should we move this higher up?

hmm, is there a point in checking for the terminator conditions if the operation is not part of some control flow (i.e., in a block and region?

I moved the condition up and then got verification errors from things like this.
I can fix those tests, but I'm unsure what we want here.

@math-fehr I ended up moving the condition for has_trait(IsTerminator) one indentation level since indeed there was an implicit assumption that a terminator must have successors, which is indeed not the case.

But my point regarding control flow (i.e., not belonging to a block/region) still stands; or am I missing something?

Hmm that's a good question. It turns out that you cannot jump to the entry block, so technically it is not allowed.
We don't raise an error yet, the question is where should we raise an error.
I believe we should raise it here, as it reduce the amount of corner cases we have (otherwise, we rely on another verification pass to handle that case, and I feel this is very error-prone if that makes sense).
What do you think?

I think it's a good point to "piggyback" this check here.

Hence, I'll just check separately that if an operation has successors and doesn't belong to a parent block or region, then it should fail verification.
This is regardless of the IsTerminator trait.

After this, we can continue with the current verification logic and enhance/supplement it with the follow-up NoTerminator trait patch.

tests/test_builtin_traits.py

tests/test_ir.py

math-fehr · 2023-06-08T13:34:01Z

xdsl/ir/core.py

+        ) is not None:
+            # TODO single-block regions dealt when the NoTerminator trait is
+            # implemented (https://github.com/xdslproject/xdsl/issues/1093)
+            if len(parent_region.blocks) > 1:


In the case where there is a single block, we still need to check that an operation that is not the last does not have a successor (with or without NoTerminator), right?
Or is it different since you technically are not allowed to jump to the entry block?

That is right but I was thinking it might be simpler to check for both cases when the NoTerminator trait is implemented. I guess the branching off for a non-last op is irrelevant to that, so we can just test it now.

Am I understanding correctly?

I changed my mind on this, it's simpler and more clear to check that an operation should not have successors unless it is the last operation in its parent block (regardless of the number of blocks in the parent region or the existence of the NoTerminator trait.

The PR is now updated to reflect this.

math-fehr

Nice! Thanks a lot

compor · 2023-06-08T13:37:35Z

Nice! Thanks a lot

No problem, thanks for the input; I'll address your remaining feedback and ping you again.
Also fighting a failing filecheck, so it might take a while.

compor · 2023-06-09T07:24:54Z

Regarding the failing filecheck tests/filecheck/parser-printer/graph_region.mlir I think the problem is that TestOp does not have the IsTerminator trait now and although the test checks operand dependency cycles and other value reference tests it fails.

My idea here was to add a TestTermOp in the test dialect with implements also the IsTerminator trait.
What do you think?

superlopuh · 2023-06-09T10:01:20Z

Seems that graph regions broke in the tests

compor · 2023-06-09T11:05:46Z

Seems that graph regions broke in the tests

I commented on this above, although now I think this is not what we might want.

It seems the graph regions are limited to a single basic block, so this might be a clue towards a solution.
Currently, looking at mlir-opt source ans tests on this.

math-fehr · 2023-06-09T12:07:27Z

Oh, I see what happened!
I think you are right, we should have a test.termop for this test.
Concerning graph regions, yes they are only for single-block regions, but here the test was testing that we don't have to order basic blocks.

…ccessors

…k or region

…ess of number of blocks in a region

compor · 2023-06-09T15:38:14Z

@math-fehr I think I have addressed all comments now.

codecov · 2023-06-09T15:38:25Z

Codecov Report

Patch coverage: 98.31% and project coverage change: +0.05 🎉

Comparison is base (8dcfb40) 86.95% compared to head (8066cbc) 87.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1080      +/-   ##
==========================================
+ Coverage   86.95%   87.00%   +0.05%     
==========================================
  Files         137      137              
  Lines       20676    20774      +98     
  Branches     3128     3143      +15     
==========================================
+ Hits        17979    18075      +96     
- Misses       2179     2180       +1     
- Partials      518      519       +1

Impacted Files	Coverage Δ
xdsl/traits.py	`93.93% <86.66%> (-6.07%)`	⬇️
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/cf.py	`100.00% <100.00%> (ø)`
xdsl/dialects/func.py	`94.61% <100.00%> (ø)`
xdsl/dialects/scf.py	`89.65% <100.00%> (+0.12%)`	⬆️
xdsl/dialects/test.py	`100.00% <100.00%> (ø)`
xdsl/ir/core.py	`87.17% <100.00%> (+0.04%)`	⬆️

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

math-fehr

Nice! Thanks a lot, that looks good to me!

math-fehr · 2023-06-09T15:43:26Z

@webmiche @superlopuh, is that good for you as well?

webmiche

One thing that I don't like, though that is probably an MLIR design issue, is that the IsTerminator trait is very closely coupled into the core datastructures now. So no block can be without a terminator essentially. This makes the dependencies within the code very hard to follow and couples different parts very closely together. To me, that is particularly apparent by the fact that you added two kinds of tests in this PR.

That's all to say, nice to have this in, I think we really need this and I think we can merge. But I will reserve the right to change this coupling at some point, if we manage to design this cleaner than MLIR does.

compor · 2023-06-09T16:49:26Z

One thing that I don't like, though that is probably an MLIR design issue, is that the IsTerminator trait is very closely coupled into the core datastructures now. So no block can be without a terminator essentially. This makes the dependencies within the code very hard to follow and couples different parts very closely together. To me, that is particularly apparent by the fact that you added two kinds of tests in this PR.

That's all to say, nice to have this in, I think we really need this and I think we can merge. But I will reserve the right to change this coupling at some point, if we manage to design this cleaner than MLIR does.

I mostly agree with this.
I think combining traits with interfaces is already an improvement in the Python world.

When I started this, I wanted to implement everything in the trait's verify method; I feel this can take place with some redesign and maybe(?) additional traits.
It just feels that the overlap of cases hasn't been well thought through in the trait system.

Regardless of these deficiencies, I like the additional verification they provide.

tests/test_operation_builder.py

This PR implements the [`IsTerminator`][3] trait. It is a spinoff of the discussion [here][1] and specifically [this][2] recommendation regarding the initial (and pending) attempt to add the `NoTerminator` trait. [1]: xdslproject#1049 [2]: xdslproject#1049 (comment) [3]: https://mlir.llvm.org/docs/Traits/#terminator

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

compor self-assigned this Jun 5, 2023

compor requested review from math-fehr, webmiche and georgebisbas as code owners June 5, 2023 15:14

webmiche reviewed Jun 5, 2023

View reviewed changes

xdsl/ir.py Outdated Show resolved Hide resolved

xdsl/ir.py Outdated Show resolved Hide resolved

superlopuh reviewed Jun 5, 2023

View reviewed changes

xdsl/ir.py Outdated Show resolved Hide resolved

superlopuh reviewed Jun 5, 2023

View reviewed changes

xdsl/ir.py Outdated Show resolved Hide resolved

superlopuh reviewed Jun 5, 2023

View reviewed changes

xdsl/ir.py Outdated Show resolved Hide resolved

math-fehr reviewed Jun 5, 2023

View reviewed changes

compor mentioned this pull request Jun 7, 2023

core: Missing terminator conditions for single-block regions in operation verification #1093

Closed

compor requested review from math-fehr and superlopuh June 7, 2023 14:19

compor force-pushed the christos/core/term-trait branch from 3f2cea3 to d5fca71 Compare June 7, 2023 14:38

math-fehr reviewed Jun 8, 2023

View reviewed changes

math-fehr approved these changes Jun 8, 2023

View reviewed changes

compor requested review from math-fehr and webmiche June 9, 2023 07:25

compor added 7 commits June 9, 2023 16:33

Add IsTerminator trait

8571d56

Add terminator check in verify method for operation

32d4719

Add tests for terminator sanity checking

379168f

Add tests for IsTerminator trait

42c2991

Resolve circular dependency between traits and ir modules

47413ca

Make test condition for null objects explicit

577c195

Simplify if condition

2590e57

compor added 16 commits June 9, 2023 16:33

Add test case

7b2dcf6

Rename test case and clarify its docstring

efdd987

Fix incorrect test docstring

6822d89

Appease pyright

9a3f8d0

Move condition for IsTerminator trait so it applies to ops without su…

795859f

…ccessors

Add test case and clarify their docstrings

41f9b1b

Fix test case

30d81c9

Add IsTerminator trait to required ops in cf, scf and func dialects

e476179

Add test case

24686dd

Add terminator operation to test dialect

b4cedd5

Use test.termop terminator in graph region test

12d943c

Check for successors when operation does not belong to block or region

6c22e3d

Add test cases checking successors for operations without parent bloc…

c4d9933

…k or region

Fix operation builder tests

2bcbf72

Check block successors exist only in the last block operation regardl…

13cf98c

…ess of number of blocks in a region

Fix test case

b669515

compor force-pushed the christos/core/term-trait branch from 1cbfc6e to b669515 Compare June 9, 2023 15:34

math-fehr approved these changes Jun 9, 2023

View reviewed changes

webmiche approved these changes Jun 9, 2023

View reviewed changes

superlopuh reviewed Jun 9, 2023

View reviewed changes

tests/test_operation_builder.py Show resolved Hide resolved

superlopuh approved these changes Jun 9, 2023

View reviewed changes

Add docstrings to changed tests

8066cbc

compor merged commit 1df2278 into main Jun 9, 2023
12 checks passed

compor deleted the christos/core/term-trait branch June 9, 2023 18:19

compor mentioned this pull request Jun 13, 2023

core: Add successor branching verification #1129

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Add IsTerminator trait #1080

Core: Add IsTerminator trait #1080

compor commented Jun 5, 2023

math-fehr left a comment

math-fehr Jun 5, 2023

compor Jun 7, 2023

compor Jun 9, 2023

math-fehr Jun 9, 2023

compor Jun 9, 2023 •

edited

math-fehr Jun 8, 2023

compor Jun 9, 2023

compor Jun 9, 2023 •

edited

math-fehr left a comment

compor commented Jun 8, 2023

compor commented Jun 9, 2023

superlopuh commented Jun 9, 2023

compor commented Jun 9, 2023

math-fehr commented Jun 9, 2023

compor commented Jun 9, 2023

codecov bot commented Jun 9, 2023 •

edited

math-fehr left a comment

math-fehr commented Jun 9, 2023

webmiche left a comment

compor commented Jun 9, 2023 •

edited

Core: Add IsTerminator trait #1080

Core: Add IsTerminator trait #1080

Conversation

compor commented Jun 5, 2023

math-fehr left a comment

Choose a reason for hiding this comment

math-fehr Jun 5, 2023

Choose a reason for hiding this comment

compor Jun 7, 2023

Choose a reason for hiding this comment

compor Jun 9, 2023

Choose a reason for hiding this comment

math-fehr Jun 9, 2023

Choose a reason for hiding this comment

compor Jun 9, 2023 • edited

Choose a reason for hiding this comment

math-fehr Jun 8, 2023

Choose a reason for hiding this comment

compor Jun 9, 2023

Choose a reason for hiding this comment

compor Jun 9, 2023 • edited

Choose a reason for hiding this comment

math-fehr left a comment

Choose a reason for hiding this comment

compor commented Jun 8, 2023

compor commented Jun 9, 2023

superlopuh commented Jun 9, 2023

compor commented Jun 9, 2023

math-fehr commented Jun 9, 2023

compor commented Jun 9, 2023

codecov bot commented Jun 9, 2023 • edited

Codecov Report

math-fehr left a comment

Choose a reason for hiding this comment

math-fehr commented Jun 9, 2023

webmiche left a comment

Choose a reason for hiding this comment

compor commented Jun 9, 2023 • edited

compor Jun 9, 2023 •

edited

compor Jun 9, 2023 •

edited

codecov bot commented Jun 9, 2023 •

edited

compor commented Jun 9, 2023 •

edited