Refactor slice op for symbolic bounds and explicit input mode - #5088
Refactor slice op for symbolic bounds and explicit input mode#5088CharlieL7 wants to merge 16 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 <cursoragent@cursor.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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 🔴 |
|
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; }))
| 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])); |
| 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); |
| 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; |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5088 +/- ##
===========================================
+ Coverage 92.90% 93.23% +0.33%
===========================================
Files 603 623 +20
Lines 32526 33032 +506
===========================================
+ Hits 30217 30797 +580
+ Misses 2309 2235 -74
🚀 New features to boost your workflow:
|
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
… sym_slice_refactor
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| @@ -205,8 +205,10 @@ struct find_mul_slice_conv | |||
| auto sop = any_cast<op::slice>(i->get_operator()); | |||
There was a problem hiding this comment.
We should use i->get_operator().to_value() instead of any_cast. That would also simplify the rewrite below so we dont need std::get<int64_t>.
There was a problem hiding this comment.
Would need to do that for sop and slice_op which changes the rest of this function? As in, it doesn't seem worth the amount of changes required.
| auto xop = any_cast<op::slice>(x->get_operator()); | ||
| auto yop = any_cast<op::slice>(y->get_operator()); | ||
| return std::tie(xop.starts, xop.ends) < std::tie(yop.starts, yop.ends); | ||
| return std::tuple(to_ints(xop.starts), to_ints(xop.ends)) < std::tuple(to_ints(yop.starts), to_ints(yop.ends)); |
There was a problem hiding this comment.
[format.py] reported by reviewdog 🐶
| return std::tuple(to_ints(xop.starts), to_ints(xop.ends)) < std::tuple(to_ints(yop.starts), to_ints(yop.ends)); | |
| return std::tuple(to_ints(xop.starts), to_ints(xop.ends)) < | |
| std::tuple(to_ints(yop.starts), to_ints(yop.ends)); |
shivadbhavsar
left a comment
shivadbhavsar
left a comment
There was a problem hiding this comment.
Looks fine overall, just a few nits about compute_shape validation
| std::vector<int64_t> axes{}; | ||
| std::vector<dim_like> starts{}; | ||
| std::vector<dim_like> ends{}; | ||
| std::vector<slice_mode> mode{}; |
There was a problem hiding this comment.
should this be a set instead to disallow duplicate entries in here?
There was a problem hiding this comment.
The order matters here. We could check for duplicates in compute_shape and then throw an error.
| /// Get the attributes that are non-empty | ||
| std::array<bool, 3> get_set_attributes() const | ||
| /// Check that the inputs, attributes, and mode are valid. | ||
| void check_inputs_and_attributes(const std::vector<shape>& inputs) const |
There was a problem hiding this comment.
Can you try this degenerate case: starts = {}, ends = {}, axes = {0}, mode = {ends}
slice(data, runtime_end)
I think it might let this through. It might be better to check for each of starts, ends, and axis that:
- there is a concrete non-dynamic and non-empty value
- If 1. is not true than verify that it exists in mode, and that there is a valid runtime input associated with it
pfultz2
left a comment
pfultz2
left a comment
There was a problem hiding this comment.
Review of the slice refactor. Most of these are about the new mode attribute and the dim_like bound type; the first three are silent-wrong-answer paths rather than crashes.
One finding could not be anchored inline because the file is not part of this diff:
test/multi_target/multitarget_test.cpp:234 — this builds the NonMaxSuppression pattern make_op("slice", {{"axes", {0}}, {"starts", {0}}}) with two inputs (idx, cnt) and no mode entry. The PR updates exactly this pattern in src/onnx/parse_nonmaxsuppression.cpp and test/ref/nonmaxsuppression.cpp but misses this copy, so add_instruction now throws SLICE: number of mode entries (0) must match number of variable inputs (1) at graph-construction time. It will not show up in normal CI because test/multi_target is gated on MIGRAPHX_ENABLE_GPU AND MIGRAPHX_ENABLE_CPU AND MIGRAPHX_ENABLE_FPGA and the latter two default to Off, but it is stale as written.
| slice_desc sd = handle_sd_inputs(parser, std::move(info), args); | ||
| // If axes arg is not given, the default is all of them. | ||
| if(sd.op.axes.empty() and sd.op_args.size() <= 3) | ||
| if(sd.axes.empty() and sd.op_args.size() <= 3) |
There was a problem hiding this comment.
This default-axes fill cannot distinguish "no axes given" from "axes given as a variable input", so it fires in the latter case and the op ends up carrying both an invented concrete axes attribute and an axes input.
Slice(data{3,4}, starts=[0,1] const, ends=[2,4] const, axes=<runtime tensor len 2>): try_insert(args[3]) does not fold, so mode={axes} and sd.axes is left empty; op_args is [data, axes_var] (size 2 <= 3), so this line overwrites sd.axes with iota {0,1}. check_inputs_and_attributes then passes (all attributes length 2, one mode entry, one variable input), and use_range_based_logic() returns false because axes is now non-empty — so symbolic_compute_shape reports {2,3} from the invented axes while compute()/resolve_bounds reads the real axes {1,0} at runtime and lens_calc yields {3,2}. Wrong shape and wrong offset, no diagnostic. Pre-PR this threw SLICE: Invalid 2 input and attributes configuration.
Second manifestation: Slice(data, starts_const, ends_var, axes_var) gives op_args size 3, so axes is filled with iota, mode={ends,axes}, and symbolic_compute_shape throws the internal SLICE: Attribute sizes do not match on a valid model.
The guard wants to be not contains(sd.mode, slice_mode::axes) rather than sd.op_args.size() <= 3. The unreachable steps and variable axes is not supported check below has the same root cause — the iota fill has already made sd.axes non-empty by the time it runs.
There was a problem hiding this comment.
This comment it way too wordy. I think all it's trying to say is that it should check using the sd.mode.
| else if(set_attributes == starts_only) | ||
| // Every set attribute shares `attr_size` and defines the number of sliced axes, so each | ||
| // variable input must match it. | ||
| if(attr_size != 0 and inputs[1].lens()[0] != attr_size) |
There was a problem hiding this comment.
check_inputs_and_attributes validates lengths only — it never cross-checks mode against which attributes are actually set. That drops an invariant the old compute_two_or_more enforced ("All of starts, ends, and axes must be supplied by either their attribute or an input, but not both").
make_op("slice", {{"axes",{0}},{"starts",{0}},{"ends",{4}},{"mode",{"ends"}}}) on data{10} plus an ends parameter: attr_size is 1 from all three attributes, mode.size() == inputs.size()-1 == 1, inputs[1].lens()[0] == 1 — everything passes. use_range_based_logic() is false (ends non-empty), so symbolic_compute_shape returns lens {4} from the attribute. At eval, resolve_bounds overwrites slot 1 from the runtime input (say 7) and compute() returns an aliased argument of lens {7}, while every downstream instruction was shape-computed and memory-coloured for {4}.
Suggest rejecting a mode entry whose corresponding attribute is non-empty and not symbolic.
There was a problem hiding this comment.
I believe this is the same comment as Shiv's earlier one. Also too wordy.
| auto start_exprs = to_sym_exprs(starts); | ||
| auto end_exprs = to_sym_exprs(ends); | ||
| for(std::size_t i = 0; i < axes.size(); ++i) | ||
| dds[axes[i]] = shape::dynamic_dimension{end_exprs[i] - start_exprs[i]}; |
There was a problem hiding this comment.
This computes the extent as raw ends - starts with no clamping, but for a non-fixed axis the bounds reaching here are never normalized, so ordinary ONNX bounds produce a negative dimension.
tune_attribute returns the bound vector unchanged whenever use_len is requested and any sliced axis is not fixed (normalize_attributes.cpp:137-143), and a symbolic var dim is never fixed. The new tune_attribute_sym only runs when the attribute values serialize as objects, so plain int bounds on a symbolic axis get no clamping at all.
So x[:, :-1] on a symbolic axis (starts=[0], ends=[-1]) reaches this line as lit(-1) - lit(0) and builds dynamic_dimension{-1}. get_interval() only asserts >= 0 in a dev build; in release it wraps to ~1.8e19 and flows into max_lens/element_space/allocation. ends = INT64_MAX ("slice to the end", which is what exporters emit) does the same.
Pre-PR this configuration was rejected outright by SLICE 1_arg: slicing is not allowed on non-fixed symbolic input axis, and slice_sym_non_fixed_throws covered it. That test is now slice_sym_non_fixed_axis, which only exercises non-negative in-range bounds, and slice_sym_clamped_and_negative_bounds covers negative bounds only on a fixed sliced axis — so nothing catches this. The "bounds are trusted to be valid at runtime" note in the new test assumes a normalization that does not actually happen here.
There was a problem hiding this comment.
I think this is trying to say that we should use tune_attribute_sym for if we're slicing over a symbolic dimension as well.
| for(std::size_t axis = 0; axis < lens.size(); axis++) | ||
| { | ||
| offset += starts[axis] * strides[axis]; | ||
| offset += std::get<int64_t>(starts[axis]) * strides[axis]; |
There was a problem hiding this comment.
Removing the old if(set_attributes != all_set) MIGRAPHX_THROW("SLICE 1_arg: ...") guard makes a 1-input slice with all three attributes empty legal, which turns this axes.empty() branch from dead code into an out-of-bounds read.
make_op("slice") (no attributes) on a single input {2,3,4}: check_inputs_and_attributes skips empty attributes so attr_size stays 0, and the inputs.size() == 1 path returns early (no symbolics, empty mode). symbolic_compute_shape's guard is 0 != 0 so it passes, the axes loop never runs, and the input shape is returned unchanged. At eval compute() takes the 1-arg path into compute_offset(s), axes.empty() is true, and this loop runs over all three dimensions doing std::get<int64_t>(starts[axis]) on an empty std::vector<dim_like> — UB, aborting under _GLIBCXX_ASSERTIONS and otherwise producing a garbage aliased pointer.
Given the new starts.size() == axes.size() invariant this branch can only ever be entered with an empty starts, so it is dead-or-UB by construction — worth replacing with an assert and restoring the all-empty rejection in check_inputs_and_attributes.
There was a problem hiding this comment.
This is the same thing again as Shiv's earlier comment.
| } | ||
| return {{"norm_starts", norm_starts}, {"norm_ends", norm_ends}, {"norm_axes", norm_axes}}; | ||
| if(result[0].empty()) | ||
| result[0] = to_ints(this->starts); |
There was a problem hiding this comment.
These fallbacks call to_ints on the attribute for any slot no mode entry filled, and nothing requires a symbolic attribute to be backed by a mode entry — so this compiles cleanly and throws at inference time.
slice{axes={0}, starts={dd{n}}, ends={5}, mode={ends}} on inputs (data{8}, ends_in{1}): check_inputs_and_attributes rejects symbolic attributes only for the 1-input case, so this passes; use_range_based_logic() is false and symbolic_compute_shape yields 5 - n. At program::eval, resolve_bounds fills slot 1 from the input, finds slot 0 empty, and calls to_ints(this->starts) — std::get<int64_t> on a dynamic_dimension throws std::bad_variant_access, a raw std exception (not MIGRAPHX_THROW) reaching the user and the C API's generic handler on every inference call.
Suggest validating any_sym(starts) implies contains(mode, slice_mode::starts) (and likewise for ends) in check_inputs_and_attributes. Separately, the empty-vector fallback also silently substitutes the attribute when a legitimate variable input has length 0 — worth distinguishing "not supplied" from "supplied but empty".
There was a problem hiding this comment.
Same comment again. Basically it sounds like we want the strictest possible checks on slice inputs and modes.
| { | ||
| 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])); |
There was a problem hiding this comment.
slice_concrete_bounds() was added in simplify_dyn_ops.cpp for exactly this hazard, but the other passes that unwrap slice bounds got the mechanical std::get<int64_t> treatment with no guard. A slice carrying a symbolic bound — the shape this PR exists to create — throws std::bad_variant_access out of the middle of a compiler pass, and there is no try/catch on the pass pipeline, so it escapes program::compile with no op name or instruction context.
Unguarded sites: this one (find_nested_slice::get_axes, matcher is just slice(slice(...)) with no nargs or bound restriction), simplify_reshapes.cpp:865-866 (find_concat_slice), :1829/:1861/:1901-1902 (find_transpose_slice), and simplify_algebra.cpp:208-236 (find_mul_slice_conv), :1400-1416 (get_splits), :1791 (find_split_concat), :2509-2510 (find_split_transpose).
Worth noting get_splits specifically: its is_static_slice guard only checks vector sizes, which used to imply concrete bounds because an input-supplied bound left the vector empty. That no longer holds — a symbolic-bound slice has starts.size() == ends.size() == axes.size() because symbolic_compute_shape requires it — so the guard now lets symbolic slices straight through to std::get.
Suggest hoisting slice_concrete_bounds() somewhere shared and composing it into these matchers.
There was a problem hiding this comment.
This should just fail. These matchers haven't been updated for symbolic slice attributes.
| ins, | ||
| make_op("slice", {{"starts", starts_vec}, {"ends", ends_vec}, {"axes", axes_vec}}), | ||
| inputs.at(0)); | ||
| fold_const_input_slice(m, mr.result); |
There was a problem hiding this comment.
find_const_2in_slice and find_const_3in_slice were both converted to the mode-aware fold_const_input_slice/resolve_bounds, but find_const_4in_slice just below was left with the old positional assumption (input1=starts, input2=ends, input3=axes) and also did not get the new slice_concrete_bounds() guard.
Nothing validates that mode is unique or in the canonical order the class comment declares, so that positional assumption is unbacked. A 4-input slice with mode={ends, starts, axes} is accepted (mode.size() == 3 == inputs-1) and is handled correctly by compute(), but find_const_4in_slice folds it into slice{starts=<ends value>, ends=<starts value>, axes} — bounds swapped, silently disagreeing with what compute() would have produced.
The underlying issue is that mode is a positional vector in which duplicates and misordering are representable but unchecked: mode={starts, starts} passes every check, and resolve_bounds then writes slot 0 twice so one input is silently ignored. Either validate sorted+unique in check_inputs_and_attributes (picking up the earlier thread about a set), or make the invalid states unrepresentable — e.g. a fixed std::array<bool,3>/std::bitset<3> indexed by slice_mode, which also turns the count check into a popcount and lets mlir.cpp build its value explicitly instead of subtracting a key. Either way, route the 4-input case through fold_const_input_slice too.
| #include <migraphx/functional.hpp> | ||
| #include <migraphx/op/normalize_attribute.hpp> | ||
| #include <migraphx/serialize.hpp> | ||
| #include <migraphx/sym.hpp> |
There was a problem hiding this comment.
Unrelated to this hunk, but this file's licence header (line 4) still reads Copyright (c) 2015-2025 while the file gains ~340 lines in this PR. Every other touched file in the diff was bumped to 2015-2026; this is the only one missed, and the stamp check in CI will fail on it.
|
Converted to draft to redesign into a new PR |
|
Refactored into #5112 |
Motivation
sliceoperator'scompute_shape()to be able to output symbolic shapes.Technical Details
Added
slice_modeSlice 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.