Refactor slice op for symbolic bounds and explicit input mode#5088
Draft
CharlieL7 wants to merge 5 commits into
Draft
Refactor slice op for symbolic bounds and explicit input mode#5088CharlieL7 wants to merge 5 commits into
CharlieL7 wants to merge 5 commits into
Conversation
Rework the slice operator to support symbolic (dim_like) start/end bounds and an explicit slice_mode selecting which of starts/ends/axes are supplied as runtime inputs (replacing the implicit get_set_attributes mechanism): - op/slice.hpp: dim_like starts/ends, slice_mode enum, symbolic_compute_shape, range-based fallback for variable inputs, runtime normalization via normalize_starts_ends_axes, attribute-consistency check, and operator<< for slice_mode. - parse_slice.cpp: set slice_mode via the new slice_input_flags bit-flag enum (enum.hpp); dim_like.hpp adds to_ints/to_sym_exprs/any_sym; normalize_attributes adds symbolic bound normalization. - Adapt slice consumers to the dim_like/mode interface: simplify_dyn_ops, simplify_algebra, simplify_reshapes. - Minimal consumer adaptation for existing multi-input slice call sites (parse_topk var-k, parse_nonmaxsuppression dynamic, parse_split dynamic): set mode=ends_input/starts_input/starts_ends_input; behavior unchanged. - Tests: op_shape_test slice cases, ref/slice, simplify_dyn_ops_test, gpu/dyn_slice_lowering, onnx slice_var_input_*, enum.cpp; plus mode updates to topk_var_k_test, ref/nonmaxsuppression, split_dyn_input. Extracted from the proto_data_dependent_symbolics branch. Excludes the topk op rework, NMS symbolic rework, and bind_symbolic removal. Co-authored-by: Cursor <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
format.py
[format.py] reported by reviewdog 🐶
AMDMIGraphX/test/op_shape_test.cpp
Lines 5179 to 5183 in 9616dac
[format.py] reported by reviewdog 🐶
AMDMIGraphX/test/op_shape_test.cpp
Lines 5190 to 5194 in 9616dac
[format.py] reported by reviewdog 🐶
AMDMIGraphX/test/op_shape_test.cpp
Lines 5202 to 5206 in 9616dac
[format.py] reported by reviewdog 🐶
AMDMIGraphX/test/op_shape_test.cpp
Lines 5214 to 5218 in 9616dac
[format.py] reported by reviewdog 🐶
AMDMIGraphX/test/op_shape_test.cpp
Lines 5226 to 5230 in 9616dac
[format.py] reported by reviewdog 🐶
AMDMIGraphX/test/ref/slice.cpp
Lines 90 to 94 in 9616dac
[format.py] reported by reviewdog 🐶
AMDMIGraphX/test/ref/slice.cpp
Lines 120 to 124 in 9616dac
[format.py] reported by reviewdog 🐶
AMDMIGraphX/test/ref/slice.cpp
Lines 262 to 266 in 9616dac
[format.py] reported by reviewdog 🐶
AMDMIGraphX/test/ref/slice.cpp
Lines 327 to 331 in 9616dac
[format.py] reported by reviewdog 🐶
AMDMIGraphX/test/ref/slice.cpp
Lines 359 to 363 in 9616dac
[format.py] reported by reviewdog 🐶
AMDMIGraphX/test/simplify_dyn_ops_test.cpp
Lines 537 to 542 in 9616dac
Regressions detected 🔴 |
|
CharlieL7
marked this pull request as ready for review
July 23, 2026 20:34
CharlieL7
marked this pull request as draft
July 23, 2026 20:35
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the core slice operator to support symbolic (dim_like) bounds, introduces an explicit mode to disambiguate variable slice inputs (starts/ends/axes), and updates ONNX parsing + tests to use the new model. It also extends attribute normalization to handle symbolic bounds by producing symbolic clamp expressions when possible.
Changes:
- Add
op::slice::slice_mode/modeattribute to explicitly define what each trailing slice input represents, and update ONNX parsers/tests accordingly. - Refactor
slicebounds todim_likesocompute_shape()can produce symbolic output shapes when bounds are symbolic. - Extend
normalize_attributes()to symbolically clampdim_likebounds (whenuse_lennormalization is requested).
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/simplify_dyn_ops_test.cpp | Updates simplify-dyn-ops slice folding tests to set the new mode attribute. |
| test/ref/slice.cpp | Updates ref backend slice tests to use mode for multi-input slice variants. |
| test/ref/nonmaxsuppression.cpp | Updates NMS ref test to set mode for the dynamic slice on outputs. |
| test/op_shape_test.cpp | Updates/extends slice shape inference tests for symbolic and variable-input modes. |
| test/onnx/parse/topk_var_k_test.cpp | Updates TopK var-K ONNX parse test to use mode for dynamic slicing. |
| test/onnx/parse/split_dyn_input.cpp | Updates dynamic split ONNX parse test to set slice mode consistently. |
| test/onnx/parse/slice_var_input_static1.cpp | Updates slice var-input static ONNX test to set mode. |
| test/onnx/parse/slice_var_input_static0.cpp | Updates slice var-input static ONNX test to set mode. |
| test/onnx/parse/slice_var_input_dyn1.cpp | Updates slice var-input dynamic ONNX test to set mode. |
| test/onnx/parse/slice_var_input_dyn0.cpp | Updates slice var-input dynamic ONNX test to set mode. |
| test/onnx/parse/slice_var_input_default_steps.cpp | Updates default-steps slice var-input ONNX test to set mode. |
| test/gpu/dyn_slice_lowering.cpp | Updates GPU lowering test to set slice mode for runtime inputs. |
| src/simplify_reshapes.cpp | Adjusts reshape simplifications to account for dim_like bounds (but needs guarding for symbolic bounds). |
| src/simplify_dyn_ops.cpp | Refactors constant-input slice folding using mode, and prevents folding when bounds aren’t concrete. |
| src/simplify_algebra.cpp | Updates algebraic rewrites that inspect slice bounds to use dim_like (but needs guarding for symbolic bounds). |
| src/onnx/parse_topk.cpp | Updates TopK parser to set slice mode for the dynamic output slice. |
| src/onnx/parse_split.cpp | Updates Split parser to set slice mode for dynamic split slices. |
| src/onnx/parse_slice.cpp | Refactors Slice parser to build op::slice with dim_like bounds and a mode vector. |
| src/onnx/parse_nonmaxsuppression.cpp | Updates NMS parser to set slice mode for the dynamic output slice. |
| src/normalize_attributes.cpp | Adds symbolic normalization path for dim_like bounds (clamp/index normalization). |
| src/include/migraphx/op/slice.hpp | Implements slice_mode, dim_like bounds, and symbolic compute-shape behavior. |
| src/include/migraphx/dim_like.hpp | Adds helpers (to_ints, to_sym_exprs, any_sym) for working with dim_like vectors. |
Comments suppressed due to low confidence (2)
src/simplify_reshapes.cpp:866
std::get<int64_t>onop::slice::{starts,ends}will throw if the slice uses symbolic bounds. This pass should skip such slices instead of throwing.
auto sop = any_cast<op::slice>(sins->get_operator());
size_t slice_start = std::get<int64_t>(sop.starts.front());
size_t slice_len = std::get<int64_t>(sop.ends.front()) - slice_start;
src/simplify_algebra.cpp:1400
get_splits()now sorts/compares starts/ends viato_ints(...)/std::get<int64_t>(...), but it only checks vector sizes—not that the bounds are actually concrete ints. With symbolic bounds (dim_likeholdingdynamic_dimension), these conversions will throw. Tightenis_static_sliceto also require non-symbolic bounds.
std::sort(result.begin(), result.end(), [&](auto x, auto y) {
return to_ints(get_start(x)) < to_ints(get_start(y));
});
if(std::any_of(get_start(result.front()).begin(),
get_start(result.front()).end(),
[&](const auto& i) { return std::get<int64_t>(i) != 0; }))
Comment on lines
+104
to
+105
| // ONNX Slice can have up to 5 inputs, we first check the 5th one | ||
| // to decide whether MIGX can handle this slice. |
Comment on lines
+196
to
200
| if(contains(mode, slice_mode::axes)) | ||
| { | ||
| // all 4 inputs (data, inputs_starts, input_ends, input_axes) | ||
| std::transform(dds.begin(), dds.end(), dds.begin(), [](const auto& dd) { | ||
| return shape::dynamic_dimension{0, dd.get_interval().max}; | ||
| }); |
Comment on lines
663
to
+667
| auto op = any_cast<op::slice>(ins->get_operator()); | ||
| for(std::size_t i = 0; i < op.axes.size(); i++) | ||
| { | ||
| result[op.axes[i]] = std::make_pair(op.starts[i], op.ends[i]); | ||
| result[op.axes[i]] = | ||
| std::make_pair(std::get<int64_t>(op.starts[i]), std::get<int64_t>(op.ends[i])); |
Comment on lines
+1824
to
+1829
| std::transform(op.ends.begin(), | ||
| op.ends.end(), | ||
| op.starts.begin(), | ||
| result.begin(), | ||
| [](const auto& e, const auto& s) { | ||
| return std::get<int64_t>(e) - std::get<int64_t>(s); |
Comment on lines
+208
to
212
| if(std::max(std::get<int64_t>(sop.starts.front()), | ||
| std::get<int64_t>(slice_op.starts.front())) < | ||
| std::min(std::get<int64_t>(sop.ends.front()), | ||
| std::get<int64_t>(slice_op.ends.front()))) | ||
| return true; |
| slice_desc construct_slice_desc(const onnx_parser& parser, | ||
| onnx_parser::node_info info, | ||
| std::vector<instruction_ref> args) const | ||
| slice_desc handle_sd_inputs(const onnx_parser& parser, onnx_parser::node_info info, std::vector<instruction_ref> args) const |
Contributor
There was a problem hiding this comment.
[format.py] reported by reviewdog 🐶
Suggested change
| slice_desc handle_sd_inputs(const onnx_parser& parser, onnx_parser::node_info info, std::vector<instruction_ref> args) const | |
| slice_desc handle_sd_inputs(const onnx_parser& parser, | |
| onnx_parser::node_info info, | |
| std::vector<instruction_ref> args) const |
| mm->add_instruction(migraphx::make_op("slice", {{"axes", {1}}}), add1, starts1, ends1); | ||
| auto s1 = mm->add_instruction( | ||
| migraphx::make_op("slice", | ||
| {{"axes", {1}}, {"mode", migraphx::value::array{"starts", "ends"}}}), |
Contributor
There was a problem hiding this comment.
[format.py] reported by reviewdog 🐶
Suggested change
| {{"axes", {1}}, {"mode", migraphx::value::array{"starts", "ends"}}}), | |
| {{"axes", {1}}, {"mode", migraphx::value::array{"starts", "ends"}}}), |
| ends0); | ||
| auto s2 = mm->add_instruction( | ||
| migraphx::make_op("slice", | ||
| {{"axes", {1}}, {"mode", migraphx::value::array{"starts", "ends"}}}), |
Contributor
There was a problem hiding this comment.
[format.py] reported by reviewdog 🐶
Suggested change
| {{"axes", {1}}, {"mode", migraphx::value::array{"starts", "ends"}}}), | |
| {{"axes", {1}}, {"mode", migraphx::value::array{"starts", "ends"}}}), |
| mm->add_instruction(migraphx::make_op("slice", {{"axes", {1}}}), add1, starts1, ends1); | ||
| auto s1 = mm->add_instruction( | ||
| migraphx::make_op("slice", | ||
| {{"axes", {1}}, {"mode", migraphx::value::array{"starts", "ends"}}}), |
Contributor
There was a problem hiding this comment.
[format.py] reported by reviewdog 🐶
Suggested change
| {{"axes", {1}}, {"mode", migraphx::value::array{"starts", "ends"}}}), | |
| {{"axes", {1}}, {"mode", migraphx::value::array{"starts", "ends"}}}), |
| ends0); | ||
| auto s2 = mm->add_instruction( | ||
| migraphx::make_op("slice", | ||
| {{"axes", {1}}, {"mode", migraphx::value::array{"starts", "ends"}}}), |
Contributor
There was a problem hiding this comment.
[format.py] reported by reviewdog 🐶
Suggested change
| {{"axes", {1}}, {"mode", migraphx::value::array{"starts", "ends"}}}), | |
| {{"axes", {1}}, {"mode", migraphx::value::array{"starts", "ends"}}}), |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5088 +/- ##
===========================================
- Coverage 92.90% 92.87% -0.04%
===========================================
Files 603 603
Lines 32526 32587 +61
===========================================
+ Hits 30217 30262 +45
- Misses 2309 2325 +16
🚀 New features to boost your workflow:
|
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
sliceoperator'scompute_shape()to be able to output symbolic shapes.Technical Details
Added
slice_modeMIGRAPHX_BIT_FLAG_ENUMtoenum.hppto have a way to create type-safe named bit flags.Slice symbolic shapes
slicefor handling symbolic shapes. Example:slice.compute()is called, the value atends_inputis used to determine the output shape.Slice range-based dynamic shapes
{0, max_of_interval}until we remove range-based dynamic shapes.Other
Changelog Category
Add a
CHANGELOG.mdentry for any option other thanNot ApplicableFollow the LLVM AI Tool Use Policy for contributions using AI.