dialects: (builtin) Fix printing and parsing of UnitAttr
#2744
Conversation
superlopuh
requested review from
AntonLydike,
georgebisbas,
math-fehr,
PapyChacal,
Moxinilian and
webmiche
xdsl/parser/attribute_parser.py
Outdated
| @@ -1250,6 +1250,7 @@ def _parse_optional_integer_or_float_type(self) -> Attribute | None: | |||
| Parse as integer or float type, if present. | |||
| integer-or-float-type ::= index-type | integer-type | float-type | |||
| index-type ::= `index` | |||
| unit-attribute ::= `unit` | |||
There was a problem hiding this comment.
I first tried to add this as a normal attribute not type, but then the test failed, saying "type expected". I have a supsicion it's actually a type in MLIR also, but my quick search for the definition didn't yield much so I thought I'd open a PR anyway.
There was a problem hiding this comment.
It is not a type, it's an attribute though: https://mlir.llvm.org/docs/Dialects/Builtin/#unitattr
There was a problem hiding this comment.
Should I change the test instead? When I put the code for parsing into parse_attribute it fails, as the block argument expects a type.
There was a problem hiding this comment.
Which test? And yes, we should change it I think
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2744 +/- ##
==========================================
- Coverage 89.78% 89.77% -0.01%
==========================================
Files 370 370
Lines 47617 47626 +9
Branches 7299 7301 +2
==========================================
+ Hits 42751 42758 +7
- Misses 3730 3731 +1
- Partials 1136 1137 +1 ☔ View full report in Codecov by Sentry. |
xdsl/parser/attribute_parser.py
Outdated
| # Unit type | ||
| if name == "unit": | ||
| self._consume_token() | ||
| return UnitAttr() | ||
|
|
There was a problem hiding this comment.
This is not the right place to add this though, this is neither an integer nor attribute type?
This reverts commit 5e83ef9.
| @@ -8,6 +8,9 @@ | |||
|
|
|||
| // CHECK: (index) | |||
|
|
|||
| "func.func"() ({}) {function_type = () -> (), sym_name = "unit_attr_func", unitarray = [unit]} : () -> () | |||
There was a problem hiding this comment.
Can I suggest using the test dialect here? I don't think we want to depend on func here...
There was a problem hiding this comment.
I agree in general, although here it seemed like a good fit for consistency, happy to change it if you or anyone else insist
| """ | ||
| if self._current_token.kind != Token.Kind.BARE_IDENT: | ||
| return None | ||
| name = self._current_token.span.text |
There was a problem hiding this comment.
supernit: Not sure this extra variable assignment is needed here?
There was a problem hiding this comment.
definitely not, although it doesn't really hurt either
It's actually
unitnot the empty string.