Skip to content

Fix dynamic shape conversion semantics #4828

Open
shivadbhavsar wants to merge 5 commits intodevelopfrom
to_symbolic_helper
Open

Fix dynamic shape conversion semantics #4828
shivadbhavsar wants to merge 5 commits intodevelopfrom
to_symbolic_helper

Conversation

@shivadbhavsar
Copy link
Copy Markdown
Contributor

Motivation

Goal here is to fix the discrepancies left after integrating symbolic shapes.

  1. Preserve original meaning of "to_dynamic" shape method before symbolic integration. For symbolic shape, simply demote to a range-based shape
  2. Add a "to_symbolic" shape method to generate a symbolic shape from a static shape by using literal expressions
  3. Add a "to_dynamic" static method that returns all inputs shape with a uniform representation

Technical Details

  1. for shape to_dynamic(): Revert back to logic before [AIMIGRAPHX-835] integrate symbolic expression in dynamic_dimension #4702. Add block to demote symbolic to pure range-based via interval computation
  2. shape to_symbolic(): Convert static shape to symbolic literals. Identity for symbolic shapes. Throws for range-based shapes
  3. static vector to_dynamic: Convert all input shapes to symbolic shapes when possible. If there is a range-based shape in the inputs, convert/demote everything to range-based shapes

In practice, there should be no mix between range-based dynamic shapes and symbolic shapes (all dynamic shapes will be represented throughout a model exclusively in one format), the goal here is to have a clear semantic anyway. The static to_dynamic function should be used in compute shapes to create a uniform representation in compute_shape methods of operators.

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 28, 2026
Copilot AI review requested due to automatic review settings April 28, 2026 21:19
@shivadbhavsar shivadbhavsar requested a review from causten as a code owner April 28, 2026 21:19
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

This PR refines MIGraphX shape conversion semantics after symbolic shape integration, clarifying when shapes should be represented as range-based dynamic vs symbolic dynamic and adding utilities to enforce a uniform representation.

Changes:

  • Reworked shape::to_dynamic() to return a range-based dynamic shape (and demote symbolic shapes by evaluating intervals/optimals), restoring pre-symbolic semantics.
  • Added shape::to_symbolic() and static shape::to_dynamic(std::vector<shape>) to promote/demote shapes to a uniform representation across inputs (including tuple sub-shapes).
  • Extended shape tests for to_symbolic, static to_dynamic, same_lens with symbolic shapes, and is_fixed().

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/shape.cpp Implements updated to_dynamic(), adds to_symbolic(), adds static to_dynamic(vector), and updates same_lens() via new is_fixed().
src/include/migraphx/shape.hpp Declares new conversion APIs and adds fixed-dim utilities plus is_fixed() declaration.
test/shape_test.cpp Updates existing expectations and adds coverage for symbolic promotion/demotion and fixed/same-lens behaviors.

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

codecov Bot commented Apr 28, 2026

Codecov Report

❌ Patch coverage is 96.07843% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/shape.cpp 96.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4828      +/-   ##
===========================================
- Coverage    92.80%   92.79%   -0.00%     
===========================================
  Files          584      584              
  Lines        30067    30111      +44     
===========================================
+ Hits         27901    27941      +40     
- Misses        2166     2170       +4     
Files with missing lines Coverage Δ
src/include/migraphx/shape.hpp 91.36% <ø> (ø)
src/sym.cpp 97.11% <100.00%> (+<0.01%) ⬆️
src/shape.cpp 91.71% <96.00%> (-0.07%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

2 participants