[AIMIGRAPHX-1215] add runtime symbol resolution op - #5085
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new resolve_sym_expr runtime operator to evaluate symbolic dimension expressions from concrete runtime dimension values, enabling dynamic-shape lowering patterns (e.g., feeding symbolic slice bounds as runtime inputs).
Changes:
- Added
op::resolve_sym_expr(reflected attributesexprs/symbols) that returns a tuple of evaluated int64 scalars. - Added reference and shape tests for
resolve_sym_expr, plus an integration-style ref test demonstrating use with dynamic multi-inputslice. - Registered the new operator in the build system.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/include/migraphx/op/resolve_sym_expr.hpp |
New operator implementation for runtime symbol expression evaluation. |
src/CMakeLists.txt |
Registers resolve_sym_expr in register_migraphx_ops(). |
test/ref/resolve_sym_expr.cpp |
New ref tests covering single/multi-symbol evaluation behavior. |
test/op_shape_test.cpp |
New shape tests (valid shape + bad input arity) for the operator. |
test/ref/slice.cpp |
New ref test demonstrating resolve_sym_expr feeding a runtime slice bound. |
Regressions detected 🔴 * No develop baseline was found for this PR's branch point; compared against the latest available develop run instead. |
|
shivadbhavsar
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5085 +/- ##
===========================================
- Coverage 93.26% 93.25% -0.01%
===========================================
Files 623 624 +1
Lines 33097 33152 +55
===========================================
+ Hits 30866 30915 +49
- Misses 2231 2237 +6
🚀 New features to boost your workflow:
|
CharlieL7
left a comment
CharlieL7
left a comment
There was a problem hiding this comment.
Design looks good. I have questions about some things.
| {{"expressions", | ||
| migraphx::value::array{migraphx::to_value(n - migraphx::sym::lit(1))}}}), | ||
| x); | ||
| mm->add_instruction(migraphx::make_op("slice", {{"axes", {2}}, {"starts", {0}}}), x, end_vals); |
There was a problem hiding this comment.
This is using the range-based compute_shape() of slice. Where the compute_shape() of slice would output the {0, max_interval} shape. To use the symbolic version you would need to add {"ends", {expr(n - 1)}} to the attributes.
There was a problem hiding this comment.
Thats true, but I need your PR to go in before I can do that I think
| #include "test.hpp" | ||
| #include "rob.hpp" | ||
|
|
||
| struct can_eval_finalize_passthrough |
There was a problem hiding this comment.
What's the reason for this logic?
There was a problem hiding this comment.
just a test to make sure const-folding doesnt try and fold an on op that requires finalization. This is just a dummy op testing that logic.
For the eval expr op, calling eval before finalize is problematic because the input_shape attribute does not exist yet
| MIGRAPHX_THROW("EVAL_EXPR_FROM_SHAPE: input shapes not captured; op was not finalized"); | ||
|
|
||
| std::unordered_map<sym::expr, std::size_t> values; | ||
| for(std::size_t i = 0; i < input_shapes.size(); ++i) |
There was a problem hiding this comment.
Can you use for(const auto&[input_shape, arg]:views::zip(input_shapes, args)) instead?
| if(not input_shape.symbolic()) | ||
| continue; | ||
| const auto& dims = input_shape.dyn_dims(); | ||
| for(std::size_t axis = 0; axis < dims.size(); ++axis) |
There was a problem hiding this comment.
same here, you can use for(const auto&[dim, len]:views::zip(dims, lens))
| { | ||
| auto s = as_symbol(x); | ||
| if(seen_variables.insert(s).second) | ||
| result.push_back(std::move(s)); |
There was a problem hiding this comment.
Do you need to push_back here? You could copy it to the vector after the traversal is finished: return {seen_variables.bgin(), seen_variables.end()}
There was a problem hiding this comment.
this just makes it so its always in the order they are encountered. But i dont think thats really relevant, Im going to change it to just return a set instead
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/include/migraphx/op/eval_expr_from_shape.hpp:114
- Result allocation should use the
output_shapepassed intocompute()rather than reconstructing a new packed shape. This keeps output layout consistent with what the framework computed incompute_shape().
argument result{shape{shape::int64_type, {expressions.size()}}};
src/include/migraphx/op/eval_expr_from_shape.hpp:52
input_shapesis derived runtime state captured infinalize(), but it is currently included inreflect(). This makes the op's serialized/hashed form depend on mutable finalized state and duplicates information already present in the instruction inputs. Consider reflecting only the user-providedexpressionsattribute and leavinginput_shapesas internal cache.
template <class Self, class F>
static auto reflect(Self& self, F f)
{
return pack(f(self.expressions, "expressions"), f(self.input_shapes, "input_shapes"));
}
src/include/migraphx/op/eval_expr_from_shape.hpp:87
- The
computesignature currently discards the providedoutput_shapeparameter, which makes it harder to ensure the producedargumentmatches the framework-provided output layout. Name the parameter and use it when constructing the result argument.
This issue also appears on line 114 of the same file.
argument compute(const shape&, std::vector<argument> args) const
src/include/migraphx/op/eval_expr_from_shape.hpp:59
- PR description says this op takes exactly one tensor input, but the implementation explicitly allows multiple inputs (
has_at_least(1)) and the tests cover cross-input symbol binding. Either the PR description should be updated, or the op should be restricted to a single input if that was the intent.
shape compute_shape(const std::vector<shape>& inputs) const
{
check_shapes{inputs, *this, true}.has_at_least(1);
src/CMakeLists.txt:222
- The PR description checks the "Added" changelog category, but there doesn't appear to be a
CHANGELOG.mdentry for the neweval_expr_from_shapeop (no mention found inCHANGELOG.mdunder Develop/Added). Please add a short entry so the change is discoverable in release notes.
elu
equal
erf
eval_expr_from_shape
exp
| const auto& dims = input_shape.dyn_dims(); | ||
| for(auto&& [dim, len] : views::zip(dims, lens)) | ||
| { | ||
| if(dim.sym_expr.name() != "variable") |
There was a problem hiding this comment.
It seems like it should be dim.sym_expr.name() == "literal" as we can replace in arbitrary expression.
Motivation
For dynamic shapes, the runtime needs to be able to evaluate symbols using concrete input dimensions to be able to compute dynamic ops (eg. slice)
Technical Details
Attributes (
std::vector<sym::expr>, reflected directly):expressions— symbolic expressions to evaluate.Inputs: one tensor whose original compile-time shape contains the referenced root symbols as direct dimensions.
At runtime, each direct symbolic dimension is bound to the corresponding concrete input lens:
Output: one packed
int64vector of shape{expressions.size()}. Elementiiseval(expressions[i]), unclamped.Implementation:
compute_shaperequires one input, verifies every referenced root symbol appears as a direct input dimension, and returnsint64_type, {expressions.size()}.computereads the original input shape fromdyn_output::input_shapesand the concrete dimensions from the runtime argument shape.eval_uint.Restriction: compound input dimensions are not inverted. For example, an input dimension
N/2cannot be used to recoverN;Nmust appear directly on an input axis.Example:
expressions=[N, H/2, W/2], compile-time input{N,3,H,W}, runtime input{7,3,10,12}→[7,5,6].Usage
Changelog Category
Add a
CHANGELOG.mdentry for any option other thanNot ApplicableFollow the LLVM AI Tool Use Policy for contributions using AI.