dialects: (arith) fix arith.constant construction
#3916
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3916 +/- ##
=======================================
Coverage 90.20% 90.20%
=======================================
Files 458 458
Lines 58353 58356 +3
Branches 5694 5692 -2
=======================================
+ Hits 52637 52642 +5
Misses 4325 4325
+ Partials 1391 1389 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Constraints currently only deal with verification and inference for declarative assembly format |
xdsl/dialects/arith.py
Outdated
| value = cast(IntegerAttr | FloatAttr[AnyFloat], value) | ||
| value_type = value.type | ||
| elif isinstance(value, DenseIntOrFPElementsAttr): | ||
| value_type = value.type |
There was a problem hiding this comment.
You should use value.get_type() to get the type associated to a TypedAttribute
xdsl/dialects/arith.py
Outdated
|
|
||
| if isinstance(value, IntegerAttr): | ||
| value_type = cast(IntegerType, value_type) | ||
| value = IntegerAttr(value.value, value_type) |
There was a problem hiding this comment.
I am against doing a silent coercion here. Personally I think we should either throw an error, or not allow a type in the constructor.
There was a problem hiding this comment.
I am in favour of not allowing the type, as this is what is the most clear IMHO. I didn't do it as I wasn't sure of the initial rationale for this API and this provides the minimum change against it.
As for throwing an error, I don't think we are doing it anywhere in any constructor.
There was a problem hiding this comment.
I believe the original API allowed passing in python ints? I would be in favour of a single constructor that takes a single attribute and infers the type from that attribute.
There was a problem hiding this comment.
The original API? I'm confused, the diff is clear, no? I can't see what you're saying. We have the from_int_and_width for that.
There was a problem hiding this comment.
Sorry, I meant original as in "when arith.constant was first added to xDSL" original.
There was a problem hiding this comment.
Well, I can't answer this, and at the end of the day that was changed to the current one that uses a typed attribute and a type (for which I mentioned I don't know the rationale behind it, AFAIU when I checked a while back, it was another deprecation of similar helpers to from_int_and_width, e.g. from_attr).
There was a problem hiding this comment.
I think at this point we can just remove value_type from the argument
There was a problem hiding this comment.
There seem to be uses within the stencil part of xDSL that use the general Attribute overload and rely on passing a value_type; for the rest of the types the API is simpler without requiring a value_type provided, as discussed previously here.
tests/dialects/test_arith.py
Outdated
| assert c1.value.type == i32 | ||
|
|
||
| c2 = ConstantOp(IntegerAttr(1, i32), i64) | ||
| assert c2.value.type == i64 |
There was a problem hiding this comment.
Is there a use case where this behaviour is desirable
There was a problem hiding this comment.
What behaviour exactly?
There was a problem hiding this comment.
Passing in an typed attribute and a different type, and having the constructor make a new attribute with the new type
There was a problem hiding this comment.
There are some obscure uses (one in CSL).
There was a problem hiding this comment.
I'm not sure it makes sense to support this
There was a problem hiding this comment.
patched the ones that were left (in experimental)
|
I am personally against this constructor, because I really don't understand why this should be a standard constructor on |
Yeah, I agree, as I said above to Alex. This is the natural thing to do, in my opinion. |
|
I agree with Mathieu :) |
Great, consensus has been reached :) I'll adjust this PR as discussed above |
I bumped into this recently, and it is documented in #2390, but it is not the parsing and printing that is broken.
We just use the
value_typewhen provided, but don't adjust the type of the value attribute to it, ending up in havingwhich is invalid IR.
I'd expect theTypedAttributeConstraintto kick in, but even though I tried to play with it, I'm not sure if I'm missing something obvious there. Happy to close this if I did.