Skip to content

Sym concat slice#4896

Open
shivadbhavsar wants to merge 4 commits into
developfrom
sym_concat_slice
Open

Sym concat slice#4896
shivadbhavsar wants to merge 4 commits into
developfrom
sym_concat_slice

Conversation

@shivadbhavsar
Copy link
Copy Markdown
Contributor

Motivation

Update the slice, concat, and step ops to support symbolic compute shape.

Technical Details

The support for slice is currently limited to fixed dimensions same as the current range-based implementation supported. It can be enhanced once we have a dim-like variant (introduced with the reshape op) that can be used for slices also.

Changelog Category

Add a CHANGELOG.md entry for any option other than Not Applicable

    • Added: New functionality.
    • Changed: Changes to existing functionality.
    • Removed: Functionality or support that has been removed. (Compared to a previous release)
    • Optimized: Component performance that has been optimized or improved.
    • Resolved Issues: Known issues from a previous version that have been resolved.
    • Not Applicable: This PR is not to be included in the changelog.

@shivadbhavsar shivadbhavsar self-assigned this May 19, 2026
@shivadbhavsar shivadbhavsar requested a review from causten as a code owner May 19, 2026 21:01
Copilot AI review requested due to automatic review settings May 19, 2026 21:01
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 extends MIGraphX’s shape inference for slice, concat, and step to work with symbolic shapes (and improves some dynamic-shape behavior), so these ops can participate correctly in symbolic compute_shape flows.

Changes:

  • Update slice 1-arg normalize_compute_shape to support symbolic shapes (while keeping the existing restriction that sliced symbolic axes must be fixed) and to preserve non-sliced dynamic dimension metadata.
  • Update step to compute symbolic output dims/strides (including symbolic stride scaling) and switch evaluation to use dyn_output.
  • Update concat to unify static/range-dynamic/symbolic shape inference and support symbolic concatenation along the concat axis.
  • Add extensive shape tests for symbolic and dynamic behavior across slice, step, and concat.

Reviewed changes

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

File Description
test/op_shape_test.cpp Adds coverage for symbolic and dynamic shape inference for slice, step, and concat (including stride/permutation preservation checks).
src/include/migraphx/op/step.hpp Enables symbolic/dynamic compute_shape for step, computes symbolic dims/strides, and uses dyn_output for runtime shape handling.
src/include/migraphx/op/slice.hpp Improves 1-arg slice dynamic handling (preserve metadata on untouched dims) and adds symbolic-shape support with fixed-axis restriction.
src/include/migraphx/op/concat.hpp Reworks concat shape inference to operate on a unified dynamic/symbolic representation and support symbolic concat-axis sizing.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

❌ Patch coverage is 98.27586% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/include/migraphx/op/slice.hpp 94.74% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4896   +/-   ##
========================================
  Coverage    92.88%   92.88%           
========================================
  Files          587      587           
  Lines        30331    30333    +2     
========================================
+ Hits         28170    28172    +2     
  Misses        2161     2161           
Files with missing lines Coverage Δ
src/include/migraphx/op/concat.hpp 100.00% <100.00%> (ø)
src/include/migraphx/op/step.hpp 93.33% <100.00%> (+0.74%) ⬆️
src/include/migraphx/op/slice.hpp 95.51% <94.74%> (+0.16%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

check_shapes{inputs, *this, true}.has(1, 2, 3, 4);
if(inputs.size() == 1)
if(inputs.size() != 1)
return compute_two_or_more(inputs);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not yet handling the 2+ slice input versions, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ya, not handling true runtime computed shapes using symbolics yet.

Comment thread test/op_shape_test.cpp
const migraphx::shape& sym_out) {
EXPECT(sym_out.to_static(sym_map) == op.compute_shape({sin.to_static(sym_map)}));
};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This doesn't support slicing a symbolic dimension?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not yet because that will require upgrading the attributes to allow symbolics

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.

3 participants