Skip to content

[AIMIGRAPHX-926] Support commonly used shape methods for symbolic shapes#4760

Closed
shivadbhavsar wants to merge 21 commits intosym_dim_integrationfrom
sym_shape_methods
Closed

[AIMIGRAPHX-926] Support commonly used shape methods for symbolic shapes#4760
shivadbhavsar wants to merge 21 commits intosym_dim_integrationfrom
sym_shape_methods

Conversation

@shivadbhavsar
Copy link
Copy Markdown
Contributor

Motivation

Commonly used methods in shape.cpp and permutation.cpp can now be used for dynamic shapes with symbols.

Technical Details

  • Add intervals to symbols
  • Use intervals to compute inequalities assuming monotonicity and linearity (reasonable assumption for dim/stride computations)
  • The expr inequality checks have a caveat where it is easy to hit boundary cases where the inequality doesn't hold strictly
    • See comment above stride sort method
    • Use interval max to perform sorting operations rather than direct sym::expr inequalities

Changelog Category

Add a CHANGELOG.md entry for any option other than Not Applicable

    • Added: New functionality.
    • Changed: Changes to existing functionality.
    • Removed: Functionality or support that has been removed. (Compared to a previous release)
    • Optimized: Component performance that has been optimized or improved.
    • Resolved Issues: Known issues from a previous version that have been resolved.
    • Not Applicable: This PR is not to be included in the changelog.

@shivadbhavsar shivadbhavsar self-assigned this Apr 9, 2026
Copilot AI review requested due to automatic review settings April 9, 2026 01:52
@shivadbhavsar shivadbhavsar requested a review from causten as a code owner April 9, 2026 01:52
@shivadbhavsar shivadbhavsar marked this pull request as draft April 9, 2026 01:53
@shivadbhavsar shivadbhavsar changed the base branch from develop to sym_dim_integration April 9, 2026 01:54
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds interval-aware symbolic expressions and extends shape/permutation utilities so commonly used shape methods work with symbolic dynamic shapes.

Changes:

  • Add bounded symbols (min/max) to sym::expr, plus eval_min()/eval_max() and interval-based semantic comparisons.
  • Add symbolic dynamic strides to shape, enabling packed/standard/transposed/broadcasted, to_static(symbol_map), from_permutation/with_lens, and permutation logic to operate on symbolic shapes.
  • Expand unit tests to cover symbolic equality/hashing, min/max evaluation, comparisons, serialization, and shape/permutation behavior.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
test/sym_test.cpp Adds coverage for bounded symbols, interval evaluation, comparisons, and serialization.
test/shape_test.cpp Adds extensive symbolic-shape tests for layout flags, permutations, compatibility, and dynamic_dimension arithmetic.
test/serialize_program.cpp Adds msgpack roundtrip test for programs containing symbolic shapes.
src/targets/gpu/hip_gemm_impl.cpp Minor batch-shape construction refactor for GEMM validation.
src/targets/gpu/gemm_impl.cpp Minor batch-shape construction refactor for GEMM validation.
src/sym.cpp Implements bounded symbols, interval eval, and semantic comparison; updates hashing/ordering/serialization.
src/shape.cpp Introduces symbolic dyn_strides and updates shape methods/serialization to support symbolic shapes.
src/permutation.cpp Adds permutation logic for symbolic shapes using max-interval evaluation.
src/include/migraphx/sym.hpp Extends public API for bounded vars, min/max eval, and comparisons.
src/include/migraphx/shape.hpp Extends dynamic_dimension and shape APIs for symbolic expressions and dyn_strides.
Comments suppressed due to low confidence (3)

src/include/migraphx/shape.hpp:139

  • dynamic_dimension::normalize_sym() is called in the constructors, but from_value() for reflectable types assigns fields directly and will not call normalize_sym() after deserialization. Older serialized shapes without the new sym field will deserialize fixed dimensions with sym_expr == nullopt, breaking the new invariant that fixed dims become symbolic constants. Consider adding a custom from_value/migraphx_from_value for dynamic_dimension (or a post-deserialize hook) that calls normalize_sym() after populating fields.
        template <class Self, class F>
        static auto reflect(Self& self, F f)
        {
            return pack(f(self.min, "min"),
                        f(self.max, "max"),
                        f(self.optimals, "optimals"),
                        f(self.sym_expr, "sym"));
        }

        bool is_fixed() const;
        bool is_symbolic() const { return sym_expr.has_value(); }
        void normalize_sym()
        {
            if(is_fixed() and not is_symbolic())
                sym_expr = sym::lit(min);
        }

src/shape.cpp:1083

  • dynamic_dimension::operator+= saturates max on overflow but updates min with unchecked min + x.min. If min overflows (wraps) while max saturates, the resulting range can become invalid and propagate incorrect bounds. Consider applying the same overflow handling to min (and similarly for operator*= where min multiplies without overflow checks).
shape::dynamic_dimension& shape::dynamic_dimension::operator+=(const shape::dynamic_dimension& x)
{
    auto lhs_sym = sym_expr;
    auto rhs_sym = x.sym_expr;
    min          = min + x.min;
    max          = (max > std::numeric_limits<std::size_t>::max() - x.max)
                       ? std::numeric_limits<std::size_t>::max()
                       : max + x.max;

src/shape.cpp:1139

  • dynamic_dimension::operator*= uses a checked/saturating multiply for max but multiplies min without overflow checks (min = min * x.min). This can overflow and produce an incorrect (wrapped) lower bound. Consider using the same safe_mul logic for min (or a dedicated safe multiply for lower bounds).
shape::dynamic_dimension& shape::dynamic_dimension::operator*=(const shape::dynamic_dimension& x)
{
    auto lhs_sym  = sym_expr;
    auto rhs_sym  = x.sym_expr;
    min           = min * x.min;
    auto safe_mul = [](std::size_t a, std::size_t b) -> std::size_t {
        if(b == 0)
            return 0;
        if(a > std::numeric_limits<std::size_t>::max() / b)
            return std::numeric_limits<std::size_t>::max();
        return a * b;
    };
    max = safe_mul(max, x.max);
    if(x.is_fixed())

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/sym.cpp
Comment thread src/sym.cpp
Comment thread src/shape.cpp
Comment thread src/shape.cpp
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 9, 2026

Codecov Report

❌ Patch coverage is 94.49275% with 19 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/sym.cpp 91.73% 11 Missing ⚠️
src/shape.cpp 96.26% 7 Missing ⚠️
src/permutation.cpp 93.33% 1 Missing ⚠️
Additional details and impacted files
@@                   Coverage Diff                   @@
##           sym_dim_integration    #4760      +/-   ##
=======================================================
+ Coverage                92.49%   92.52%   +0.02%     
=======================================================
  Files                      583      583              
  Lines                    29670    29888     +218     
=======================================================
+ Hits                     27443    27651     +208     
- Misses                    2227     2237      +10     
Files with missing lines Coverage Δ
src/include/migraphx/shape.hpp 92.00% <100.00%> (+0.45%) ⬆️
src/include/migraphx/sym.hpp 100.00% <100.00%> (ø)
src/permutation.cpp 75.56% <93.33%> (+4.97%) ⬆️
src/shape.cpp 92.40% <96.26%> (+1.10%) ⬆️
src/sym.cpp 95.89% <91.73%> (-0.70%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread src/sym.cpp
// expected.
// 3. The number of unique symbols per expression is small (typically
// 1-3), making the 2^k evaluation cost negligible.
std::pair<int64_t, int64_t> expr::compute_bounds() const
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think this is a good way to do this. #4782 already can do this with interval math so it works for more operators(like subtraction) and doesnt require 2^k evaluations.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I use this as a quick and easy approach for now till the updated symbolic expr library goes in with better interval arithmetic

Comment thread src/shape.cpp Outdated
{
auto bounds = sym_expr->compute_bounds_uint();
min = bounds.first;
max = bounds.second;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Computing min and max at every step might be slow. We should first refactor the codebase to use get_interval() and get_optimals():

struct dynamic_dimension
{
    struct interval
    {
        std::size_t min;
        std::size_t max;
    };

    interval get_interval() const;
    std::set<std::size_t> get_optimals() const;
};

We can probably use cursor/claude to do the refactor. At first it can just store the member variables like what we are doing now and then later we can change it to compute this from the symbolic expression.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am looking into doing this and I think it is a good idea. Only thing is, it will make #4702 kind of pointless (which is also ok). Would you @pfultz2 and @CharlieL7 be ok to review this mega PR with all the changes then? and i would discard 4702

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you first just do the refactor to get_interval and get_optimals with no functional changes? I can review that PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure: #4784

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So given that struct, can we settle on how we would lazily compute the intervals and optimals? My thinking atm is to make the range and optimals fields optional (they would always be defied for current range-based shapes) that for symbolic shapes would only populate values when some compute_intervals method is called (throw if some codepath calls min() or max() before explicitly making this call). Does that sound reasonable or did you have something different in mind?

Copy link
Copy Markdown
Collaborator

@pfultz2 pfultz2 Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking we store the interval as std::optional<interval> for when we are using the old intervals. Then if we are using symbolic expression we would compute it on the fly:

interval get_interval() const {
    if(range)
        return *range;
    sym::interval v = e.eval_interval(); 
    return {.max = to<std::size_t>(v.max), .min = to<std::size_t>(v.max); }
}

Later when we remove all the interval code we would get rid of the field and it just do e.eval_interval().

@shivadbhavsar
Copy link
Copy Markdown
Contributor Author

Abandoning PR, and adding updates after requested refactor straight to #4702

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants