Symbolic attr support for slice#5043
Open
shivadbhavsar wants to merge 10 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds support for representing slice attribute bounds (starts/ends) as symbolic expressions by introducing a dim_like variant type, enabling shape inference for input-size–dependent dynamic slicing.
Changes:
- Update
op::sliceto storestarts/endsasstd::vector<dim_like>and add a unified symboliccompute_shapepath. - Teach ONNX parsing and attribute normalization to preserve symbolic (object-serialized) bounds while still clamping purely-concrete bounds.
- Extend shape tests to cover non-fixed symbolic axes and symbolic start/end bounds.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| test/op_shape_test.cpp | Adds shape-inference coverage for symbolic slice bounds, clamping, and negative indices. |
| src/simplify_reshapes.cpp | Updates reshape/slice simplifications to extract concrete bounds from dim_like. |
| src/simplify_dyn_ops.cpp | Updates dynamic-op simplifications to convert slice bounds from dim_like to int64_t where required. |
| src/simplify_algebra.cpp | Updates algebraic rewrites that inspect slice ranges to handle dim_like bounds. |
| src/onnx/parse_slice.cpp | Updates Slice parser to populate dim_like starts/ends (including attribute forms). |
| src/normalize_attributes.cpp | Skips compile-time clamping for symbolic (object-encoded) bounds during attribute normalization. |
| src/include/migraphx/op/slice.hpp | Implements symbolic shape computation for slice and migrates bounds storage to dim_like. |
| src/include/migraphx/dim_like.hpp | Adds dim_like utilities, including to_ints() for extracting concrete bounds. |
Comment on lines
+292
to
+300
| if(input_shape.dynamic() and not input_shape.symbolic()) | ||
| { | ||
| MIGRAPHX_THROW("SLICE 1_arg: slicing is not allowed on non-fixed dynamic input axis "); | ||
| } | ||
|
|
||
| auto new_lens = lens_calc(input_shape.max_lens(), this->starts, this->ends, this->axes); | ||
|
|
||
| if(not input_shape.dynamic()) | ||
| return shape{input_shape.type(), new_lens, input_shape.strides()}; | ||
| if(std::any_of(axes.begin(), axes.end(), [&](auto axis) { | ||
| return not input_shape.dyn_dims()[axis].is_fixed(); | ||
| })) | ||
| MIGRAPHX_THROW("SLICE 1_arg: slicing is not allowed on a non-fixed input axis "); | ||
|
|
||
| auto dds = input_shape.dyn_dims(); | ||
| for(auto axis : this->axes) | ||
| { | ||
| dds[axis] = input_shape.symbolic() | ||
| ? shape::dynamic_dimension{sym::lit(new_lens[axis])} | ||
| : shape::dynamic_dimension{new_lens[axis], new_lens[axis]}; | ||
| auto new_lens = lens_calc(input_shape.max_lens(), to_ints(starts), to_ints(ends), axes); | ||
| auto dds = input_shape.dyn_dims(); |
Regressions detected 🔴 * No develop baseline was found for this PR's branch point; compared against the latest available develop run instead. |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5043 +/- ##
===========================================
- Coverage 92.90% 92.88% -0.02%
===========================================
Files 603 603
Lines 32526 32624 +98
===========================================
+ Hits 30217 30301 +84
- Misses 2309 2323 +14
🚀 New features to boost your workflow:
|
CharlieL7
reviewed
Jul 9, 2026
Comment on lines
+5332
to
+5343
| TEST_CASE(slice_sym_non_fixed_axis) | ||
| { | ||
| // Slicing on a non-fixed symbolic axis is rejected (same contract as range). | ||
| auto n = var("n", {1, 8}); | ||
| migraphx::shape sin{migraphx::shape::float_type, {dd{lit(4)}, dd{n}, dd{lit(8)}}}; | ||
| throws_shape(migraphx::make_op("slice", {{"axes", {1}}, {"starts", {0}}, {"ends", {2}}}), sin); | ||
| // Slicing a non-fixed symbolic axis is allowed: the bounds are trusted to be valid at | ||
| // runtime, so the extent is just ends - starts (no clamping). The other symbol is kept. | ||
| auto n = var("n", {1, 8}); | ||
| auto m = var("m", {1, 8}); | ||
| std::unordered_map<se, std::size_t> sym_map = {{n, 5}, {m, 3}}; | ||
| migraphx::shape sin{migraphx::shape::float_type, {dd{n}, dd{m}}}; | ||
| auto op = migraphx::make_op("slice", {{"axes", {0}}, {"starts", {0}}, {"ends", {2}}}); | ||
| migraphx::shape sout{migraphx::shape::float_type, {dd{lit(2)}, dd{m}}, sin.dyn_strides()}; | ||
| expect_shape(sout, op, sin); | ||
| EXPECT(sout.to_static(sym_map) == op.compute_shape({sin.to_static(sym_map)})); |
Collaborator
There was a problem hiding this comment.
This doesn't look correct. If the starts and ends are not normalized, we can't use the ends - starts. If both starts and ends are positive, we can compute a bound. But if the starts is negative it gets a lot more uncertain.
CharlieL7
requested changes
Jul 9, 2026
| if(input_shape.dynamic() and std::any_of(axes.begin(), axes.end(), [&](auto axis) { | ||
| return not input_shape.dyn_dims()[axis].is_fixed(); | ||
| })) | ||
| if(input_shape.dynamic() and not input_shape.symbolic()) |
Collaborator
There was a problem hiding this comment.
I handle this differently for ranged-based dynamics: #5015
CharlieL7
reviewed
Jul 9, 2026
Comment on lines
+5413
to
+5414
| // The sliced extent (ends - starts) must be non-negative across the whole variable | ||
| // range, so each var range is chosen to keep end >= start. |
Collaborator
There was a problem hiding this comment.
I'm not sure this is a good assumption. It's basically stating that starts and ends must be valid for the whole range of possible values. What happens when the valid slices are a subset of the possibilities: starts = var("s" , {0, 4}), ends = var("e", {2, 8})?
CharlieL7
reviewed
Jul 14, 2026
| if(std::holds_alternative<shape::dynamic_dimension>(d)) | ||
| return std::get<shape::dynamic_dimension>(d).sym_expr; | ||
| return sym::lit(std::get<int64_t>(d)); | ||
| } |
Collaborator
There was a problem hiding this comment.
Based on our recent conversation, I think this compute_shape() needs to be able to take in the symbolic map at eval time to resolve the dim_like attribute?
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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
For input-size based dynamic slicing, we can represent the slice size using a symbolic expression
Technical Details
Use the newly added dim-like variant to support symbolic starts and ends for slice
Changelog Category
Add a
CHANGELOG.mdentry for any option other thanNot Applicable