xdsl: DictionaryAttr for the builtin dialect #258
Conversation
Codecov ReportBase: 89.35% // Head: 89.20% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #258 +/- ##
==========================================
- Coverage 89.35% 89.20% -0.16%
==========================================
Files 49 49
Lines 6672 6770 +98
Branches 1118 1136 +18
==========================================
+ Hits 5962 6039 +77
- Misses 482 495 +13
- Partials 228 236 +8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
georgebisbas
changed the title
xdsl/dialects/builtin.py
Outdated
| @staticmethod | ||
| def parse_parameter(parser: Parser) -> dict[_DictionaryAttrT]: | ||
| parser.parse_char("{") | ||
| data = parser.parse_list(parser.parse_optional_attribute) |
There was a problem hiding this comment.
Could you add the tests for this parsing/printing method?
Here, this should be parse_dictionary instead of parse_list, so this needs to be changed.
| printer.print_string("}") | ||
|
|
||
| @staticmethod | ||
| def generic_constraint_coercion(args: tuple[Any]) -> AttrConstraint: |
There was a problem hiding this comment.
Here, we either need to add the correct coercion, or raise an exception.
You can see how to do this by looking at how ArrayAttr is doing it.
xdsl/dialects/builtin.py
Outdated
| f"Wrong type given to attribute {self.name}: got" | ||
| f" {type(self.data)}, but expected dictionary of" | ||
| " attributes") | ||
| for idx, val in enumerate(self.data): |
There was a problem hiding this comment.
Here, you should check that both the types of the keys and the values.
Thus, you should use self.data.items() instead of enumerate(...)
xdsl/dialects/builtin.py
Outdated
|
|
||
|
|
||
| @irdl_attr_definition | ||
| class DictionaryAttr(GenericData[dict[_DictionaryAttrT]]): |
There was a problem hiding this comment.
What about the key types?
I think you want this to be a dict[str, _DictionaryAttrT] instead.
xdsl/parser.py
Outdated
| def parse_dict_entry(self, parse_optional_one: Callable[[], T | None]): | ||
| # Limitation currently is that the key is a string | ||
| key = self.parse_str_literal() | ||
| if key is not None: |
There was a problem hiding this comment.
key should never be None here.
xdsl/parser.py
Outdated
| res = res | entry | ||
| return res | ||
|
|
||
| def parse_dict_entry(self, parse_optional_one: Callable[[], T | None]): |
There was a problem hiding this comment.
Can you add the return type to the function?
| @@ -460,6 +460,35 @@ def parse_list(self, | |||
| res.append(one) | |||
| return res | |||
|
|
|||
| def parse_dictionary(self, | |||
There was a problem hiding this comment.
Can we reuse the code that we are currently using to parse the dictionary attribute?
| @@ -122,6 +123,17 @@ def print_list(self, | |||
| self.print(delimiter) | |||
| print_fn(elem) | |||
|
|
|||
| def print_dictionary(self, | |||
There was a problem hiding this comment.
Same here, can we reuse the code we already have for the attribute dictionary in operations?
# Conflicts: # tests/filecheck/mlir-conversion/builtin_attrs.mlir # xdsl/parser.py
Provides the DictionaryAttr for the builtin dialect which is required for FIR dialect