dialects: (arith) make parsing robust with respect to block ordering #1305

tobiasgrosser · 2023-07-20T06:12:22Z

The function Parse.parse_operand only works, if we are certain that all operands are defined before they are used. As arith can be used in arbitrary control flow graphs, we need to use the robust pattern of Parser.parse_unresolved_operand + Parser.resolve_operands.

Going forward we will remove remaining uses of parse_operand and eventually rename parse_unresolved_operand to parse_operand.

The function Parse.parse_operand only works, if we are certain that all operands are defined before they are used. As arith can be used in arbitrary control flow graphs, we need to use the robust pattern of Parser.parse_unresolved_operand + Parser.resolve_operands. Going forward we will remove remaining uses of parse_operand and eventually rename parse_unresolved_operand to parse_operand.

math-fehr

👍

tobiasgrosser · 2023-07-20T06:20:58Z

tests/xdsl_opt/test_xdsl_opt.py

@@ -156,7 +157,7 @@ def test_diagnostic_exception():

    opt = xDSLOptMain(args=[filename_in])

-    with pytest.raises(DiagnosticException):
+    with pytest.raises(ParseError):


This change is needed, as the Parser does now the type-checking and now throws an error:

operand is used with type f32, but has been previously used or defined with type i32

Is there a specific reason that we do type-checking in the parser. I feel this is somehow the wrong place to do type checking as it e.g. prevents further type errors at other locations to be emitted.

Let me check if we should instead change the program that is being parsed there.

The reason this is checked at parse time and not after, is because the program cannot even be constructed.
If you refer to a value that is expected to have type i32, and then you parse that value definition which has a different type, let's say i64, then you have a fundamental inconsistency in your program.
It's not even that it is ill-typed, it is that a value has two different types, and thus we should signal it directly.

I am confused. In which way would a type error be fundamentally inconsistent?

What I mean is that you cannot even create an IR that correspond to that program, because you have a value that has two different types.

Also, note that this is different than having an operation not verifying. This is handled later.

Also, this is exactly how it is done in MLIR, just in case.

codecov · 2023-07-20T06:21:39Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (fd1bc58) 89.61% compared to head (286c911) 89.62%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1305      +/-   ##
==========================================
+ Coverage   89.61%   89.62%   +0.01%     
==========================================
  Files         187      187              
  Lines       24560    24565       +5     
  Branches     3691     3691              
==========================================
+ Hits        22010    22017       +7     
+ Misses       1968     1967       -1     
+ Partials      582      581       -1

Impacted Files	Coverage Δ
xdsl/dialects/arith.py	`96.73% <100.00%> (+0.01%)`	⬆️
xdsl/dialects/test.py	`100.00% <100.00%> (ø)`

... 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 · 2023-07-20T06:27:41Z

I quickly fixed the test so it still check the same thing as before, which is that a unverified program will return a diagnostic error.

tests/xdsl_opt/unverified_program.mlir

superlopuh

thank you

webmiche

LGTM, modulo using the test dialect.

webmiche · 2023-07-20T08:21:01Z

tests/filecheck/dialects/arith/arith_cfg.mlir

@@ -0,0 +1,32 @@
+// RUN: xdsl-opt %s


Could we use the test dialcet here?

Also is this really a property of arith or just a property of the ir-structure in general? If so, we could propably replace everything with the test dialect and move this to a new folder "ir-structure" or something.

No, we can unfortunately not. This test case tests the custom parsing of the arith.add operation, so we need at least arith to stick around. Or are you suggesting I replace the func.func and cf.br operation with the test dialect? I guess that could be possible.

Yes, I think the func and cf dialects could certainly go.

Ah yes, I see the changes happen within arith, so that one can't go 👍

The test.termop does not allow > 0 successors. Are you suggesting we expand termop to allow this?

Yes, that would be needed too, I guess. IMO, you can do this in this PR, I don't mind. It feels like a rather small change...

I tried this, but the build breaks as I cannot get var_successor_def to work.

@webmiche, can you see if you can get the test dialect fixed here?

Fixed it. Let's see what Mathieu thinks about my fixes and then we can merge, I guess.

AntonLydike

LGTM, modulo some pyright issues it seems?

webmiche · 2023-07-20T13:55:02Z

xdsl/dialects/test.py

@@ -34,7 +36,7 @@ class TestOp(IRDLOperation):


 @irdl_op_definition
-class TestTermOp(TestOp):
+class TestTermOp(IRDLOperation):


@math-fehr it seems that this was broken before. This would only collect the constraints of the parent op for verification and not the ones specified within this class. Do we want it to be that way? Or did I just bandage over an IRDL bug?

I remember doing this, but TBH, I didn't think it might be a potential bug.
My intention was to have TestTermOp be exactly like TestOp plus the terminator trait.

Yes, I remember the PR and I think I also thought it was a good idea at the time.

I think this is a problem of decorators and the order they are called in. I think we want this to work on an API level, as it feels very natural (as you say "a test op plus the terminator trait"), but it seems to not work atm. Depending on Mathieu's answer, I would suggest opening an issue or not :)

Seems like a 🐛 to me yes!

opened #1312

webmiche · 2023-07-20T13:55:36Z

xdsl/dialects/test.py

@@ -46,6 +48,10 @@ class TestTermOp(TestOp):

    name = "test.termop"

+    res: VarOpResult = var_result_def()


Does it make sense for a terminator to have results?
This is something we used in some of the tests. That's why my earlier try failed (CI run)

the particular semantics of terminator operations is determined by the specific dialect operations involved

from the MLIR doc.

Looking at the MLIR repo (test suite and lib) there is no terminator with results that I could find.
I'm not sure if there is a point, but it's not explicitly disallowed.

Terminators propagate their operands to their successors unless a value def dominates a block in the region (except when IsolatedFromAbove), in which case it is just accessible.

TL;DR: I think you can, but not sure if it's of any use.

My question was also about SSAValue scoping which I was not 100% sure about anymore. If SSAValues are block-scoped, this does not make sense as the terminator is the last operation in the block. If they are region-scoped (which from a quick google search, they seem to be in MLIR), I guess you could build IR that uses it.

And then the follow up question is: Will there ever be a use for it? Who would define a terminator that defines an SSAValue?

I guess we can leave it as-is. Better test more behavior than what people actually use. But, it was something that made me raise my eyebrow, so I decided to comment on it 😅

I guess you could build IR that uses it.

Yeah, true, I missed that point.
But my doubts remain on the utility of this, and when in Rome, I'd rather do what MLIR does (or does not do in this case :-)

Technically, a terminator can have regions, and even results.
Should they? I don't think so, but maybe someone one day will have a use case for it, so we can keep it as MLIR does.

I added regions as well. Let's keep it like this...

math-fehr

👍

math-fehr · 2023-07-20T14:53:16Z

xdsl/dialects/test.py

@@ -46,6 +48,10 @@ class TestTermOp(TestOp):

    name = "test.termop"

+    res: VarOpResult = var_result_def()


Technically, a terminator can have regions, and even results.
Should they? I don't think so, but maybe someone one day will have a use case for it, so we can keep it as MLIR does.

webmiche · 2023-07-20T15:34:47Z

I think we can merge after a rebase.

) This PR performs a minor cleanup in tests (just `test_ir.py`) wrt changes in the `test` dialect from #1305: - Remove helper class `SuccessorOp` since `TestTermOp` accepts successors now - Adjust `test` dialect import and use `test.TestOp` instead of `TestOp`

tobiasgrosser requested review from math-fehr, webmiche and georgebisbas as code owners July 20, 2023 06:12

tobiasgrosser requested review from superlopuh and AntonLydike July 20, 2023 06:12

math-fehr approved these changes Jul 20, 2023

View reviewed changes

Fix formatting and pytest

b982a59

tobiasgrosser force-pushed the tobiasgrosser/arith_robust_for_cfg branch from e19bdc3 to b982a59 Compare July 20, 2023 06:17

tobiasgrosser commented Jul 20, 2023

View reviewed changes

remove DiagnosticException

87c54c4

superlopuh reviewed Jul 20, 2023

View reviewed changes

tests/xdsl_opt/unverified_program.mlir Show resolved Hide resolved

superlopuh approved these changes Jul 20, 2023

View reviewed changes

math-fehr added 2 commits July 20, 2023 08:26

Fix test

6e8358d

Put back the diagnosticerror

8d05260

superlopuh assigned tobiasgrosser Jul 20, 2023

webmiche reviewed Jul 20, 2023

View reviewed changes

Try to use termop, but still failing

9778103

AntonLydike approved these changes Jul 20, 2023

View reviewed changes

webmiche added 3 commits July 20, 2023 15:38

add type import

360e974

change parent of test.termop

3a1ce5d

add results for terminator op

52a8210

webmiche reviewed Jul 20, 2023

View reviewed changes

math-fehr approved these changes Jul 20, 2023

View reviewed changes

add regions to terminator op

9abda35

Merge branch 'main' into tobiasgrosser/arith_robust_for_cfg

286c911

webmiche merged commit 3ad7a88 into main Jul 20, 2023
10 checks passed

webmiche deleted the tobiasgrosser/arith_robust_for_cfg branch July 20, 2023 19:11

compor mentioned this pull request Jul 21, 2023

tests: (minor) Remove helper class and adjust test dialect import #1317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dialects: (arith) make parsing robust with respect to block ordering #1305

dialects: (arith) make parsing robust with respect to block ordering #1305

tobiasgrosser commented Jul 20, 2023

math-fehr left a comment

tobiasgrosser Jul 20, 2023

math-fehr Jul 20, 2023

tobiasgrosser Jul 20, 2023

math-fehr Jul 20, 2023

math-fehr Jul 20, 2023

codecov bot commented Jul 20, 2023 •

edited

math-fehr commented Jul 20, 2023

superlopuh left a comment

webmiche left a comment

webmiche Jul 20, 2023

tobiasgrosser Jul 20, 2023

webmiche Jul 20, 2023

tobiasgrosser Jul 20, 2023

webmiche Jul 20, 2023

tobiasgrosser Jul 20, 2023

tobiasgrosser Jul 20, 2023

webmiche Jul 20, 2023

AntonLydike left a comment

webmiche Jul 20, 2023

compor Jul 20, 2023 •

edited

webmiche Jul 20, 2023

math-fehr Jul 20, 2023

webmiche Jul 20, 2023

webmiche Jul 20, 2023

compor Jul 20, 2023 •

edited

webmiche Jul 20, 2023 •

edited

compor Jul 20, 2023

math-fehr Jul 20, 2023

webmiche Jul 20, 2023

math-fehr left a comment

math-fehr Jul 20, 2023

webmiche commented Jul 20, 2023

		@@ -46,6 +48,10 @@ class TestTermOp(TestOp):

		name = "test.termop"

		res: VarOpResult = var_result_def()

dialects: (arith) make parsing robust with respect to block ordering #1305

dialects: (arith) make parsing robust with respect to block ordering #1305

Conversation

tobiasgrosser commented Jul 20, 2023

math-fehr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 20, 2023 • edited

Codecov Report

math-fehr commented Jul 20, 2023

superlopuh left a comment

Choose a reason for hiding this comment

webmiche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AntonLydike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compor Jul 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compor Jul 20, 2023 • edited

Choose a reason for hiding this comment

webmiche Jul 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

math-fehr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webmiche commented Jul 20, 2023

codecov bot commented Jul 20, 2023 •

edited

compor Jul 20, 2023 •

edited

compor Jul 20, 2023 •

edited

webmiche Jul 20, 2023 •

edited