Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Broadcast with dims matcher #2927

Merged
merged 10 commits into from
Apr 11, 2024
Merged

Conversation

bpickrel
Copy link
Contributor

@bpickrel bpickrel commented Mar 26, 2024

Add a matcher to simplify_dyn_ops which replaces broadcast_with_dims (Onnx Expand) with the equivalent multibroadcast with a fixed output shape attribute.

This conversion is not just a simplification but is required for this op. to run at all since a definite output shape is needed at runtime. broadcast_with_dims has a dynamic output shape at compile time, and this matcher replaces it with a fixed value that is available at runtime.

@bpickrel bpickrel requested a review from CharlieL7 March 26, 2024 19:12
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.84%. Comparing base (5fc2121) to head (e8ffc81).

Additional details and impacted files
@@              Coverage Diff               @@
##           expand_dyn_op    #2927   +/-   ##
==============================================
  Coverage          91.83%   91.84%           
==============================================
  Files                480      480           
  Lines              18357    18369   +12     
==============================================
+ Hits               16859    16871   +12     
  Misses              1498     1498           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@CharlieL7
Copy link
Collaborator

This conversion is not just a simplification but is required for this op. to run at all since a definite output shape is needed at runtime. broadcast_with_dims has a dynamic output shape at compile time, and this matcher replaces it with a fixed value that is available at runtime.

That's not a fully correct statement. broadcast_with_dims will run fine on the ref target. This matcher is required for dynamic batch and to run on GPU.

Copy link
Collaborator

@CharlieL7 CharlieL7 left a comment

Choose a reason for hiding this comment

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

Need to update this PR to merge some changes

Comment on lines 40 to 41
* This conversion is required to be made since runtime target ops can only output
* static shapes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove

src/simplify_dyn_ops.cpp Outdated Show resolved Hide resolved
@@ -966,6 +966,30 @@ TEST_CASE(broadcast_for_dot_dyn2)
s0);
}

TEST_CASE(broadcast_with_dims0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CharlieL7 I think these two tests were from you, but Github is showing them as new to this PR. Did you mean to keep them, or not?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The tests look the same to me, just moved around.

@@ -34,6 +34,36 @@ void run_pass(migraphx::module& m)
migraphx::run_passes(m, {migraphx::simplify_dyn_ops{}, migraphx::dead_code_elimination{}});
}

TEST_CASE(broadcast_with_dims)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CharlieL7 can you think of any other tests needed for this matcher PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good as is

@bpickrel bpickrel marked this pull request as ready for review April 1, 2024 20:40
@bpickrel bpickrel requested a review from causten as a code owner April 1, 2024 20:40
@bpickrel bpickrel marked this pull request as draft April 1, 2024 20:41
@bpickrel bpickrel self-assigned this Apr 1, 2024
@bpickrel bpickrel marked this pull request as ready for review April 1, 2024 21:10
Copy link
Contributor

@lakhinderwalia lakhinderwalia left a comment

Choose a reason for hiding this comment

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

  1. Please add a negative test, with dimensions that cannot be multi-broadcasted.
  2. Approving the PR.
    Thanks.

@bpickrel bpickrel added the enhancement New feature or request label Apr 3, 2024
@bpickrel
Copy link
Contributor Author

bpickrel commented Apr 8, 2024

@CharlieL7 , Chris asked that we merge this PR into #2799 instead of waiting for that one to be merged into develop. It shouldn't cause much extra fuss, but I want to make sure you're OK before I do it. Recall that any of us has permissions to make a merge that isn't into the develop branch.

src/simplify_dyn_ops.cpp Outdated Show resolved Hide resolved
@CharlieL7
Copy link
Collaborator

@CharlieL7 , Chris asked that we merge this PR into #2799 instead of waiting for that one to be merged into develop. It shouldn't cause much extra fuss, but I want to make sure you're OK before I do it. Recall that any of us has permissions to make a merge that isn't into the develop branch.

Yeah, that's fine

@causten
Copy link
Collaborator

causten commented Apr 11, 2024

@bpickrel please fix the merge conflict

@bpickrel bpickrel merged commit f7800ac into expand_dyn_op Apr 11, 2024
37 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants