Skip to content

Remove strict stride multiple enforcement for expanded tensor support#2226

Merged
justinrosner merged 6 commits intodevelopfrom
2225-strided-dimension
Feb 4, 2026
Merged

Remove strict stride multiple enforcement for expanded tensor support#2226
justinrosner merged 6 commits intodevelopfrom
2225-strided-dimension

Conversation

@justinrosner
Copy link
Copy Markdown
Contributor

@justinrosner justinrosner commented Feb 2, 2026

Motivation

This PR removes a strict check in MIGraphXToTosa that was preventing MIGraphX from compiling certain concat kernels. This implements: https://amd-hub.atlassian.net/browse/AIROCMLIR-472

Technical Details

The AsUnderlyingShapeConverter was incorrectly rejecting valid non-contiguous output buffer strides. Specifically, it required that memory layout dimensions be integer multiples of the logical tensor dimensions. This failed for cases like <4x5x24xf16, 288x24x1> where the logical shape is 4x5x24 but the memory layout is 4x12x24. This pattern could occur when MIGraphX concatenates tensors (e.g., a 4x5x24 and 4x7x24 into a 4x12x24 buffer), where each kernel writes to a different slice of the final buffer.

Test Plan

  • PR CI

Test Result

  • PR CI

Submission Checklist

@justinrosner justinrosner requested a review from causten as a code owner February 2, 2026 16:47
Copilot AI review requested due to automatic review settings February 2, 2026 16:47
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes an overly restrictive stride validation check that was preventing MIGraphX from compiling valid concat kernels. The check required memory layout dimensions to be integer multiples of logical tensor dimensions, which failed for cases where a kernel writes to a non-contiguous slice of a concatenated output buffer.

Changes:

  • Removed the modulo check in AsUnderlyingShapeConverter that enforced integer-multiple relationship between memory layout and logical dimensions
  • Added a positive test case demonstrating 4x5x24 logical tensor with 4x12x24 memory layout (non-multiple expansion)
  • Removed the negative test case that was validating the now-removed error condition

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
mlir/lib/Conversion/MIGraphXToTosa/MIGraphXToTosa.cpp Removes 4-line modulo validation check that was rejecting valid non-contiguous output buffer strides
mlir/test/Conversion/MIGraphXToTosa/migraphx-to-tosa-non-contiguous-strides.mlir Adds positive test for non-multiple expansion (4x5x24→4x12x24) and removes obsolete negative test case

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@pfultz2
Copy link
Copy Markdown

pfultz2 commented Feb 3, 2026

I still get a similar error:

Error: 'rock.expand_strides' op output dimension 12 is not a multiple of input dimension 7

Which might come from here:

return emitOpError("output dimension ")

@justinrosner
Copy link
Copy Markdown
Contributor Author

I still get a similar error:

Error: 'rock.expand_strides' op output dimension 12 is not a multiple of input dimension 7

Which might come from here:

return emitOpError("output dimension ")

Removed that check as well. I'm assuming this is from running the second kernel?

Comment thread mlir/lib/Conversion/MIGraphXToTosa/MIGraphXToTosa.cpp
Comment thread mlir/test/Dialect/Rock/lowering-expand-strides.mlir
@justinrosner justinrosner force-pushed the 2225-strided-dimension branch from 2a2e5db to d57a7d6 Compare February 4, 2026 13:32
@justinrosner
Copy link
Copy Markdown
Contributor Author

@justinrosner justinrosner merged commit 0ad0fae into develop Feb 4, 2026
16 checks passed
@justinrosner justinrosner deleted the 2225-strided-dimension branch February 4, 2026 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants