dialect: (irdl) Add region and attributes operations #3049
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3049 +/- ##
==========================================
+ Coverage 89.86% 89.89% +0.02%
==========================================
Files 415 416 +1
Lines 52078 52232 +154
Branches 8048 8075 +27
==========================================
+ Hits 46801 46955 +154
- Misses 3983 3984 +1
+ Partials 1294 1293 -1 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
xdsl/printer.py
Outdated
| if not elems: | ||
| self.print("{}") | ||
| else: | ||
| self.print("{ ") | ||
| for i, (key, value) in enumerate(elems.items()): | ||
| if i: | ||
| self.print_string(delimiter) | ||
| print_key(key) | ||
| self.print_string(" = ") | ||
| print_value(value) | ||
| self.print(" }") |
There was a problem hiding this comment.
Just noticed this, I don't think it belongs in a PR tagged as dialects, since it's actually a core change. Why is it necessary?
There was a problem hiding this comment.
It's not "necessary", I could just put this logic in the printer for AttributesOp, but I thought it would make sense to use the print_dictionary function and I (maybe incorrectly) thought that since it was used nowhere else that it would be safe to modify/improve
There was a problem hiding this comment.
I think if we want to change this to insert newlines then the logic has to be done from a method in Printer, as the newline methods are private
There was a problem hiding this comment.
Would be happy to split this change off into a separate pr if wanted
There was a problem hiding this comment.
Hmm, that's weird that it's not used... I'm also surprised none of the tests fail... My first instinct is to remove the helper if it's not used. In either case, I would probably recommend do this in a custom way in your custom syntax.
There was a problem hiding this comment.
I have reverted the core change and put improved printing in AttributesOp
|
I simplified the printing a bit, removing the empty case as this should never occur in practice (in fact mlir does not even parse it). I also added the verifier that mlir has to check that there are the same number of names and values, is it worth adding a test for this? |
|
Just realised that mlir does parse the empty case, but does it by omitting the curly braces entirely |
Adds
RegionsOp,RegionOp,RegionType, andAttributesOpto irdl