Enhance symbolic onnx parsing via sym_eval - #5148
Conversation
Bring the PR #5112-based parser prerequisite branch onto the current development baseline before applying its remaining changes. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep the merged resolver regression compatible with the dynamic-slice interface introduced by PR #5112. Co-authored-by: Cursor <cursoragent@cursor.com>
Track exact integral shape values through parser operations so dynamic consumers retain symbolic output relationships without changing runtime dataflow. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep the symbolic-value change focused by restoring existing resolver diagnostics and simplifying the signed Gather size declaration. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
This PR extends MIGraphX’s symbolic shape machinery by introducing symbolic tensor evaluation (sym_eval / symbolic_compute) and using it in ONNX parsing and attribute normalization so shape-dependent subgraphs can preserve symbolic output shapes instead of falling back to range-dynamic shapes.
Changes:
- Add
sym_argument+operation::symbolic_compute()andinstruction::sym_eval()to enable symbolic evaluation of int64/bool “metadata tensors” (e.g., shapes, indices). - Enhance ONNX parsers (e.g., Slice/Reshape/Expand/Range/Trilu/Where/Shape) to leverage
sym_evalfor improved symbolic output-shape propagation. - Add the new
dyn_sliceoperator and symbolic attribute normalization, with broad test coverage across ref/gpu/onnx.
Reviewed changes
Copilot reviewed 68 out of 71 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/include/operation.hpp | Add sym_argument include + type-erased symbolic_compute plumbing in generator template. |
| test/sym.cpp | Add rewrite + interval/fixed-value/provable-equality tests for symbolic reasoning improvements. |
| test/sym_test.cpp | Extend sym serialization tests (bare numbers → literals) and reformat a few cases. |
| test/sym_eval_test.cpp | New unit tests for instruction::sym_eval() and symbolic compute fallback behavior. |
| test/ref/slice.cpp | Add symbolic slice coverage and update dynamic slicing test to use dyn_slice. |
| test/ref/dynamic_range.cpp | Add symbolic-output coverage for dynamic_range. |
| test/ref/dyn_slice.cpp | New reference backend tests for dyn_slice semantics and symbolic shapes. |
| test/onnx/verify/trilu_dynamic_test.cpp | New ONNX verify tests for symbolic Trilu output shapes. |
| test/onnx/verify/symbolic_shape_values_test.cpp | New ONNX verify test ensuring symbolic shape metadata is preserved and evaluated at runtime. |
| test/onnx/verify/symbolic_shape_arithmetic_test.cpp | New ONNX verify test for symbolic arithmetic propagation through shape subgraphs. |
| test/onnx/parse/symbolic_reshape_markers_test.cpp | New parse test to assert specific marker pattern for symbolic reshape handling. |
| test/onnx/gen_onnx.py | Add ONNX model generators for new symbolic shape tests. |
| test/normalize_ops_test.cpp | Add tests for symbolic normalization behavior (notably dyn_slice bounds). |
| test/normalize_attributes_test.cpp | New tests that characterize normalize_attributes integer normalization surface. |
| test/gpu/eval_expr_from_shape_lowering.cpp | New GPU lowering tests for keeping slice metadata on host and copying results when needed. |
| test/gpu/dyn_slice_lowering.cpp | New GPU lowering tests for dyn_slice runtime metadata movement/synchronization. |
| src/targets/gpu/lowering.cpp | Extend lowering to support dyn_slice and host-only eval_expr_from_shape metadata usage. |
| src/sym.cpp | Add interval::contains, fixed_value, provable_equal, folding helpers, and bare-number expr deserialization. |
| src/onnx/parse_where.cpp | Simplify Where parsing and rely on add_common_op for broadcasting. |
| src/onnx/parse_trilu.cpp | Implement symbolic-path Trilu parsing using dynamic-range + broadcasted mask construction. |
| src/onnx/parse_squeeze.cpp | Update copyright year. |
| src/onnx/parse_slice.cpp | Convert certain symbolic Slice cases into dyn_slice using sym_eval of bounds. |
| src/onnx/parse_shape.cpp | Refactor start/end normalization into a helper and tighten types. |
| src/onnx/parse_reshape.cpp | Use sym_eval to resolve reshape dims symbolically when possible (via eval_expr_from_shape). |
| src/onnx/parse_range.cpp | When possible, convert symbolic Range into dynamic_range with symbolic output dimension. |
| src/onnx/parse_generic_op.cpp | Add includes needed for new generic parsing behavior. |
| src/onnx/parse_expand.cpp | Use sym_eval to compute symbolic broadcast output dims and pass them to broadcast_with_dims. |
| src/onnx/parse_constant_of_shape.cpp | Use sym_eval to allocate with a symbolic output shape when possible. |
| src/onnx/parse_compare_op.cpp | Update copyright year. |
| src/onnx/parse_cast.cpp | Minor refactor + include fix. |
| src/onnx/include/migraphx/onnx/op_parser.hpp | Update copyright year; remove unused include. |
| src/onnx/include/migraphx/onnx/onnx_parser.hpp | Add missing includes (optional, limits, unordered_set) used by parser implementation. |
| src/normalize_attributes.cpp | Add symbolic attribute normalization for sym::expr-backed attributes and refactor integer path. |
| src/instruction.cpp | Implement instruction::sym_eval() with memoization and symbolic_compute fallback. |
| src/include/migraphx/tensor_view.hpp | Fix bounds assertions and make to_vector() usable for const T views. |
| src/include/migraphx/sym.hpp | Expose interval::contains, provable_equal, fixed_value, and fold_min/fold_max. |
| src/include/migraphx/sym_argument.hpp | New public header defining sym_argument and helpers for symbolic tensor evaluation. |
| src/include/migraphx/operators.hpp | Register dyn_slice operator header. |
| src/include/migraphx/operation.hpp | Add symbolic_compute to the public operation interface and type-erased dispatch. |
| src/include/migraphx/op/where.hpp | Add where::symbolic_compute to enable symbolic evaluation where condition is fixed. |
| src/include/migraphx/op/unsqueeze.hpp | Add unsqueeze::symbolic_compute pass-through behavior. |
| src/include/migraphx/op/unary.hpp | Add optional symbolic compute path to unary ops via CRTP flag. |
| src/include/migraphx/op/sub.hpp | Enable symbolic compute for sub. |
| src/include/migraphx/op/squeeze.hpp | Add squeeze::symbolic_compute pass-through behavior. |
| src/include/migraphx/op/slice.hpp | Clarify symbolic-shape constraints and add slice::symbolic_compute for 1D symbolic tensors. |
| src/include/migraphx/op/reshape.hpp | Add reshape::symbolic_compute pass-through behavior. |
| src/include/migraphx/op/normalize_attribute.hpp | Document symbolic normalization behavior for expression attributes. |
| src/include/migraphx/op/multibroadcast.hpp | Add multibroadcast::symbolic_compute broadcast behavior. |
| src/include/migraphx/op/mul.hpp | Enable symbolic compute for mul. |
| src/include/migraphx/op/identity.hpp | Add identity::symbolic_compute pass-through behavior. |
| src/include/migraphx/op/gather.hpp | Add limited gather::symbolic_compute (axis 0, fixed indices). |
| src/include/migraphx/op/equal.hpp | Enable symbolic compute for equal using provable_equal. |
| src/include/migraphx/op/dynamic_range.hpp | Add optional symbolic output_dim to produce symbolic output shape for Range. |
| src/include/migraphx/op/dyn_slice.hpp | New dyn_slice operator with symbolic-bound attributes + runtime-bound inputs. |
| src/include/migraphx/op/div.hpp | Enable symbolic compute for div with conservative div-by-zero rejection. |
| src/include/migraphx/op/dimensions_of.hpp | Add dimensions_of::symbolic_compute to return symbolic dimension expressions. |
| src/include/migraphx/op/convert.hpp | Enable symbolic compute for safe int64↔bool conversions. |
| src/include/migraphx/op/concat.hpp | Add limited concat::symbolic_compute for axis-0 concatenation of symbolic tensors. |
| src/include/migraphx/op/broadcast.hpp | Add broadcast::symbolic_compute broadcast behavior. |
| src/include/migraphx/op/broadcast_with_dims.hpp | Add optional out_dyn_dims attribute and validation for fully symbolic broadcast outputs. |
| src/include/migraphx/op/binary.hpp | Add optional symbolic compute path to binary ops via CRTP flag. |
| src/include/migraphx/op/add.hpp | Enable symbolic compute for add. |
| src/include/migraphx/instruction.hpp | Add instruction::sym_eval() declaration. |
| src/include/migraphx/dim_like.hpp | Add helpers for dimension lists (all_ints, to_ints) and include updates. |
| src/CMakeLists.txt | Register dyn_slice operator for build. |
| CHANGELOG.md | Document new dyn_slice operator and symbolic attribute normalization; clarify slice symbolic support. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| bool interval::contains(const scalar& value) const | ||
| { | ||
| return not scalar_less(value, min) and not scalar_less(max, value); | ||
| } |
…DMIGraphX into core_symbolic_evaluation
Regressions detected 🔴 * No develop baseline was found for this PR's branch point; compared against the latest available develop run instead. |
|
| static constexpr bool enable_symbolic_compute = false; | ||
|
|
There was a problem hiding this comment.
Why do we need this? Just for ops that inherit another op?
There was a problem hiding this comment.
yeah, not every derived op can support symbolic compute
| sym_argument symbolic_compute(const shape& output_shape, | ||
| const std::vector<sym_argument>& args) const | ||
| { | ||
| if(axis != 0 or args.empty() or any_of(args, [](const auto& arg) { return arg.empty(); })) |
There was a problem hiding this comment.
Why does axis have to be 0? Shouldn't the condition be instead that the args are 1 dimensional?
| sym_argument symbolic_compute(const shape& output_shape, | ||
| const std::vector<sym_argument>& args) const | ||
| { | ||
| if(args.size() != 1 or |
There was a problem hiding this comment.
Shouldn't there being 1 argument already be handled by compute_shape()?
| return [](auto x, auto y) { | ||
| if constexpr(std::is_same<std::decay_t<decltype(y)>, sym::expr>{}) | ||
| { | ||
| if(y.eval_interval().contains(0)) |
There was a problem hiding this comment.
Is this for handling an empty interval?
There was a problem hiding this comment.
no its handling the case where there could be a division by 0
| if(args.empty() or args[0].get_shape().dynamic() or args[0].get_shape().ndim() != 1 or | ||
| args[0].empty()) | ||
| return {}; | ||
|
|
There was a problem hiding this comment.
This is for handling slice after something like dimensions_of, right? It's somewhat unfortunate that all this handling code for normalizing the attributes needs to be redone here again.
There was a problem hiding this comment.
yeah it uses the same normalization code but i dont see how we can avoid having to call normalization altogether
| return std::nullopt; | ||
| } | ||
|
|
||
| std::optional<scalar> fixed_value(const expr& expression) |
There was a problem hiding this comment.
Is this to check that an expression has a single possible value using the interval analysis? If so, please add a short explanation comment for the function.
There was a problem hiding this comment.
ya, adding comment
| return result; | ||
| } | ||
|
|
||
| sym_argument instruction::sym_eval() const |
There was a problem hiding this comment.
Would be good to have a short comment for this function.
| { | ||
| std::vector<op_desc> operators() const { return {{"Shape"}}; } | ||
|
|
||
| static std::pair<std::size_t, std::size_t> |
There was a problem hiding this comment.
Was there a functional change here?
There was a problem hiding this comment.
no Im going to revert this. There was previously a functional change which is why I rewrote it, but thats not necessary now.
|
|
||
| #include <onnx_test.hpp> | ||
|
|
||
| TEST_CASE(symbolic_reshape_markers) |
There was a problem hiding this comment.
What is this trying to test?
There was a problem hiding this comment.
this is for when the reshape dims contain 0 and -1. But this is messy, I am rewriting it as separate and more focused tests
No description provided.