-
Notifications
You must be signed in to change notification settings - Fork 14.6k
[mlir][vector] Update representation of insert/extract_strided_slice #101850
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
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-mlir-spirv @llvm/pr-subscribers-mlir Author: Benjamin Maxwell (MacDue) ChangesThis commit updates the representation of both Because previously the offsets, strides, and sizes were in the attribute dictionary (with no special syntax), simply replacing the attribute types with So since a syntax break is mostly unavoidable this commit also tackles a long-standing TODO:
This is done by introducing a new With this:
To:
(matching the TODO) And
To:
(inspired by the TODO) Almost all test changes were done automatically via This PR is split into multiple commits to make the changes more understandable.
Patch is 354.52 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/101850.diff 44 Files Affected:
|
@llvm/pr-subscribers-backend-amdgpu Author: Benjamin Maxwell (MacDue) ChangesThis commit updates the representation of both Because previously the offsets, strides, and sizes were in the attribute dictionary (with no special syntax), simply replacing the attribute types with So since a syntax break is mostly unavoidable this commit also tackles a long-standing TODO:
This is done by introducing a new With this:
To:
(matching the TODO) And
To:
(inspired by the TODO) Almost all test changes were done automatically via This PR is split into multiple commits to make the changes more understandable.
Patch is 354.52 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/101850.diff 44 Files Affected:
|
This is the most ambitious change based on #100997. Note: This PR is not urgent! It's just something I think would be nice to have 🙂 |
✅ With the latest revision this PR passed the C/C++ code formatter. |
13457fe
to
7034b77
Compare
7034b77
to
159f822
Compare
printer << sizes[strideIdx] << ':'; | ||
printer << strides[strideIdx] << ']'; | ||
} | ||
} |
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.
There seems to be an assumption about the size of all the arrays, but I don't see the invariant enforced in a verifier?
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.
I didn't add a verifier for the attribute, as the constraints are enforced within the ops (right now). I think some of those constraints could be factored out to the StridedSliceAttr
though 👍
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.
Going to leave this for a future patch now it's not needed for the printer/parser (which just uses the current syntax).
This commit updates the representation of both extract_strided_slice and insert_strided_slice to primitive arrays of int64_ts, rather than ArrayAttrs of IntegerAttrs. This prevents a lot of boilerplate conversions between IntegerAttr and int64_t. This is done by adding a new `StridedSliceAttr` which matches the previous syntax and can be used for both operations. It may also be possible to explore alternate slice syntax for the `StridedSliceAttr` in future.
159f822
to
c5b35e2
Compare
This commit updates the representation of both extract_strided_slice and insert_strided_slice to primitive arrays of int64_ts, rather than ArrayAttrs of IntegerAttrs. This prevents a lot of boilerplate conversions between IntegerAttr and int64_t.
This is done by adding a new
StridedSliceAttr
which matches the previous syntax and can be used for both operations.It may also be possible to explore alternate slice syntax for the
StridedSliceAttr
in future.