dialects: (builtin) fix VectorType scalable dimension representation #3947
Conversation
superlopuh
requested review from
compor,
alexarice,
math-fehr,
jorendumoulin and
webmiche
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3947 +/- ##
=======================================
Coverage 91.30% 91.31%
=======================================
Files 467 468 +1
Lines 58068 58141 +73
Branches 5581 5580 -1
=======================================
+ Hits 53021 53091 +70
- Misses 3616 3623 +7
+ Partials 1431 1427 -4 ☔ View full report in Codecov by Sentry. |
xdsl/dialects/builtin.py
Outdated
| @@ -782,6 +782,27 @@ def unpack( | |||
|
|
|||
|
|
|||
| BoolAttr: TypeAlias = IntegerAttr[Annotated[IntegerType, IntegerType(1)]] | |||
| TRUE_ATTR = BoolAttr(True, i1) | |||
There was a problem hiding this comment.
I am in general against the addition of more helper methods that do the same things in questionably more ergonomic syntax. There is already BoolAttr.from_bool() and I feel we should only keep one or the other.
There was a problem hiding this comment.
I had a feeling we had such a helper but forgot where it was.
| num_scalable_dims = IntAttr(num_scalable_dims) | ||
| super().__init__([shape, element_type, num_scalable_dims]) | ||
| if scalable_dims is None: | ||
| false = BoolAttr(False, i1) |
There was a problem hiding this comment.
| false = BoolAttr(False, i1) | |
| false = BoolAttr.from_bool(False) |
xdsl/printer.py
Outdated
| (`[x]`). | ||
| """ | ||
| dim, scalable = pair | ||
| if scalable.value: |
There was a problem hiding this comment.
| if scalable.value: | |
| if scalable: |
Should work right?
xdsl/dialects/builtin.py
Outdated
|
|
||
| def get_num_dims(self) -> int: | ||
| return len(self.shape.data) | ||
|
|
||
| def get_num_scalable_dims(self) -> int: | ||
| return self.num_scalable_dims.data | ||
| # Data for BoolAttr True is -1 | ||
| return -sum(d.value.data for d in self.scalable_dims) |
There was a problem hiding this comment.
| return -sum(d.value.data for d in self.scalable_dims) | |
| return sum(bool(d) for d in self.scalable_dims) |
There was a problem hiding this comment.
| return -sum(d.value.data for d in self.scalable_dims) | |
| return sum(1 for d in self.scalable_dims if d) |
Alternative
| // RUN: xdsl-opt --print-op-generic %s | mlir-opt --mlir-print-op-generic | xdsl-opt --print-op-generic | filecheck %s | ||
|
|
||
| // CHECK: vector<1xindex> | ||
| builtin.module attributes {v.v1 = vector<1xindex>} {} |
There was a problem hiding this comment.
I'd prefer to test with "test.op"() <{ test = ATTR }> : () -> ()
superlopuh
changed the title
Fixes #3654