[AIROCMLIR-599] Lower migraphx.slice into tensor.slice#2281
Merged
[AIROCMLIR-599] Lower migraphx.slice into tensor.slice#2281
migraphx.slice into tensor.slice#2281Conversation
Member
Author
|
The models are the following, and can be found here: |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds support for lowering migraphx.slice ops to tensor.extract_slice during the MIGraphX→Linalg conversion, and updates lit tests to reflect the newly-supported operation.
Changes:
- Add a
SliceConverterto lowermigraphx.sliceintotensor.extract_slicein the MIGraphX→Linalg conversion. - Add lit coverage for slice lowering and remove
migraphx.slicefrom the “not implemented” negative test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
mlir/lib/Conversion/MIGraphXToLinalg/MIGraphXToLinalg.cpp |
Introduces SliceConverter pattern and registers it in the conversion pattern list. |
mlir/test/Conversion/MIGraphXToLinalg/mixr-to-linalg-ops.mlir |
Adds FileCheck assertions + MIGraphX slice test cases to validate lowering output. |
mlir/test/Conversion/MIGraphXToLinalg/migraphx-to-linalg-not-implemented.mlir |
Removes the expected-failure test for migraphx.slice now that it’s implemented. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
umangyadav
reviewed
Mar 13, 2026
Comment on lines
+275
to
+277
| // CHECK-DAG: %[[expanded:.*]] = tensor.expand_shape %[[arg0]] {{.*}} output_shape [10, 10] : tensor<100xf32> into tensor<10x10xf32> | ||
| // CHECK-DAG: %[[extracted_slice:.*]] = tensor.extract_slice %[[expanded]][2, 2] [8, 8] [1, 1] : tensor<10x10xf32> to tensor<8x8xf32> | ||
| // CHECK-DAG: %[[collapsed:.*]] = tensor.collapse_shape %[[extracted_slice]] {{.*}} : tensor<8x8xf32> into tensor<64xf32> |
Member
There was a problem hiding this comment.
Is CHECK-DAG necessary here ? wouldn't CHECK be better ?
Mr-Anyone
commented
Mar 16, 2026
Comment on lines
+282
to
+291
| func.func @func_slice1(%arg0: !migraphx.shaped<1x36x384x64xf32, 884736x24576x64x1>) -> !migraphx.shaped<1x12x384x64xf32, 294912x24576x64x1> attributes{kernel, arch = ""} { | ||
| %0 = migraphx.slice %arg0 {axes = [1], ends = [12], starts = [0]} : <1x36x384x64xf32, 884736x24576x64x1> -> <1x12x384x64xf32, 294912x24576x64x1> | ||
| return %0 : !migraphx.shaped<1x12x384x64xf32, 294912x24576x64x1> | ||
| } | ||
|
|
||
| // CHECK-LABEL: func.func @func_slice2 | ||
| // CHECK: tensor.extract_slice {{.*}}[0, 0, 184, 0] [1, 12, 100, 64] [1, 1, 1, 1] : tensor<1x36x384x64xf32> to tensor<1x12x100x64xf32> | ||
| func.func @func_slice2(%arg0: !migraphx.shaped<1x36x384x64xf32, 884736x24576x64x1>) -> !migraphx.shaped<1x12x100x64xf32, 76800x6400x64x1> attributes{kernel, arch = ""} { | ||
| %0 = migraphx.slice %arg0 {axes = [1, 2], ends = [12, 284], starts = [0, 184]} : <1x36x384x64xf32, 884736x24576x64x1> -> <1x12x100x64xf32, 76800x6400x64x1> | ||
| return %0 : !migraphx.shaped<1x12x100x64xf32, 76800x6400x64x1> |
Member
Author
There was a problem hiding this comment.
FYI - This testcase comes from tosa pass
Mr-Anyone
force-pushed
the
pr-migraphx-slice
branch
from
37a8cf1 to
284c020
Compare
Mr-Anyone
force-pushed
the
pr-migraphx-slice
branch
from
284c020 to
d397d5a
Compare
Mr-Anyone
force-pushed
the
pr-migraphx-slice
branch
3 times, most recently
from
fc70bcb to
2cea6ef
Compare
Mr-Anyone
force-pushed
the
pr-migraphx-slice
branch
3 times, most recently
from
f68bda2 to
d6a1e4d
Compare
justinrosner
reviewed
Apr 7, 2026
Comment on lines
+1156
to
+1157
| int64_t axisStartInt = cast<IntegerAttr>(axisS).getInt(); | ||
| int64_t axisEndInt = cast<IntegerAttr>(axisE).getInt(); |
Contributor
There was a problem hiding this comment.
Right now it looks like we are assuming that start and end are non-negative and in-bounds. ONNX semantics allows for negative indices in slicing, so check with the MIGraphX folks (or in their source code) if this is something that they support as well.
Contributor
There was a problem hiding this comment.
Also, once you have this information, can you add a verifier for the migraphx.slice op? I don't think it currently has one.
Member
Author
There was a problem hiding this comment.
Looking over the MIGraphX codebase, I believe the normalize_pass will transform those attribute into positive values.
There is also no negative values to my understanding of migraphx.slice. I am going to add the following as invariant:
- axes, start, end is the same size
- input and output is the same rank
- all attribute is non negative
- axis is small than input rank
- start is less than end
- end is less than input shape (prevent out of bound access)
- input + attribute infers output shape
Contributor
There was a problem hiding this comment.
If we don't take negative values, then just make sure that we error out when we detect those.
umangyadav
approved these changes
Apr 9, 2026
umangyadav
approved these changes
Apr 9, 2026
Member
umangyadav
left a comment
umangyadav
left a comment
There was a problem hiding this comment.
Make sure to address justin's comments before merging.
It would be good to enable one E2E test with slice
Mr-Anyone
force-pushed
the
pr-migraphx-slice
branch
from
b806ece to
ba4fcb0
Compare
justinrosner
approved these changes
Apr 9, 2026
Mr-Anyone
force-pushed
the
pr-migraphx-slice
branch
from
16a5163 to
6622c52
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
It is known that around 70 traces in fusion zoo requires the support of
migraphx.slice. Currently, 67.6% of the trace that cannot be compiled is known to havemigraphx.slicein them.Technical Details
Essentially transfer the attribute from
migraphx.sliceintotensor.extract_slice. The size attribute from tensor.extract_slice comes from subtract the end and start attribute from migraphx.slice (note that end is not included in the dimension). The stride is always one in this case.The code structure should look like the one found in
MIGraphXToTosa.cpp[here].Test Plan
Added a lit test
Test Result
Pass lit test.
Submission Checklist