-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dialects: (arith, core) Constant op to accept tensor and memref #2969
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2969 +/- ##
=======================================
Coverage 89.84% 89.85%
=======================================
Files 408 408
Lines 51002 51028 +26
Branches 7909 7912 +3
=======================================
+ Hits 45825 45850 +25
- Misses 3927 3928 +1
Partials 1250 1250 ☔ View full report in Codecov by Sentry. |
@@ -896,147 +892,6 @@ def verify(self, attr: Attribute, constraint_context: ConstraintContext) -> None | |||
constraint.verify(attr, constraint_context) | |||
|
|||
|
|||
@irdl_attr_definition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why move this down? What's the change here? Git isn't being very helpful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As briefly mentioned in the PR description, though likely not explained in sufficient detail, I had to move RankedVectorOrTensorOf
after MemRefType
, as I changed it to now also accept MemRefType
. As a follow-up, DenseIntOrFPElementsAttr
also had to be moved down since it uses RankedVectorOrTensorOf
.
As far as I remember, the only change required to DenseIntOrFPElementsAttr
was to change the signature of the type
argument in from_list
from RankedVectorOrTensorOf[AnyFloat | IntegerType | IndexType]
to:
RankedVectorOrTensorOf[AnyFloat | IntegerType | IndexType]
| RankedVectorOrTensorOf[AnyFloat]
| RankedVectorOrTensorOf[IntegerType]
| RankedVectorOrTensorOf[IndexType]
Not fully clear why, but it does make pyright happy.
BTW, accepting new name suggestions for RankedVectorOrTensorOf
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes a lot of sense, thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me, I would probably wait for someone more familiar with MLIR than I am to comment on how similar this is to how things happen over there. My understanding is that some of the names follow the MLIR API.
Mabe @math-fehr or @zero9178 can advise?
I'd appreciate that, not super familiar with that part of the API myself, but it appears |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I don't think |
It appears that |
I don't think it makes much sense either; but this is about matching MLIR's implementation, not changing it, IMO? |
It's lowered during bufferization, e.g.:
|
Upstream mlir accepts the following arith.constant:
As for tensors, xdsl currently accepts it in generic form and print it in both generic and non-generic form, but will fail to parse the non-generic form.
As for memrefs, xdsl will not accept it and requires an extensive change to the attribute parser to add support for memrefs in
RankedVectorOrTensorOf
. Some code had to be moved after the definition ofMemRefType
to support this change.