Optimize broadcast + transpose for nonscalars #2271
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was
linked to
issues
Oct 2, 2023
Codecov Report
@@ Coverage Diff @@
## develop #2271 +/- ##
========================================
Coverage 91.32% 91.33%
========================================
Files 434 434
Lines 16245 16262 +17
========================================
+ Hits 14836 14853 +17
Misses 1409 1409
|
This build is OK for merge ✅ |
🔴bert_base_cased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output🔴distilgpt2_fp16: FAILED: MIGraphX is not within tolerance - check verbose output |
kahmed10
requested review from
umangyadav,
CharlieL7 and
bpickrel
and removed request for
CharlieL7
…MIGraphX into bcast_transpose_generic
TedThemistokleous
added
enhancement
bpickrel
approved these changes
Oct 6, 2023
umangyadav
reviewed
Oct 11, 2023
Comment on lines
+681
to
+682
| auto sum = m1.add_instruction(migraphx::make_op("add"), xb, yb); | ||
| m1.add_instruction(pass_op{}, sum); |
There was a problem hiding this comment.
Includes optimizations to find_inner_broadcast because otherwise a bunch of multibroadcasts were appearing after the transformation
Do you have a test for this specific case where multibroadcasts are appearing after find_inner_broadcast that are not being cleaned up by simplify_reshapes ? because for this test it will add multibroadcast but later i think it will get cleaned up by find_nop_reshaper.
There was a problem hiding this comment.
yes, this exact test will fail currently on develop and insert a bunch of multibroadcasts:
./bin/test_simplify_algebra_test simplify_inner_broadcast_no_common_axis
[ RUN ] simplify_inner_broadcast_no_common_axis
void simplify_inner_broadcast_no_common_axis()
/code/AMDMIGraphX/test/simplify_algebra_test.cpp:686:
FAILED: m1 == m2 [ y = @param:y -> int32_type, {1, 5, 1}, {5, 1, 1}, target_id=0
x = @param:x -> int32_type, {5, 10}, {10, 1}, target_id=0
@2 = multibroadcast[out_lens={1, 5, 10},out_dyn_dims={}](x) -> int32_type, {1, 5, 10}, {0, 10, 1}, target_id=0
@3 = multibroadcast[out_lens={1, 5, 10},out_dyn_dims={}](y) -> int32_type, {1, 5, 10}, {5, 1, 0}, target_id=0
@4 = add(@2,@3) -> int32_type, {1, 5, 10}, {0, 10, 1}, target_id=0
@5 = multibroadcast[out_lens={1, 5, 10},out_dyn_dims={}](@4) -> int32_type, {1, 5, 10}, {0, 10, 1}, target_id=0
@6 = multibroadcast[out_lens={1, 5, 10},out_dyn_dims={}](@5) -> int32_type, {1, 5, 10}, {0, 10, 1}, target_id=0
@7 = multibroadcast[out_lens={1, 5, 10},out_dyn_dims={}](@6) -> int32_type, {1, 5, 10}, {0, 10, 1}, target_id=0
@8 = multibroadcast[out_lens={1, 5, 10},out_dyn_dims={}](@7) -> int32_type, {1, 5, 10}, {0, 10, 1}, target_id=0
@9 = multibroadcast[out_lens={1, 5, 10},out_dyn_dims={}](@8) -> int32_type, {1, 5, 10}, {0, 10, 1}, target_id=0
@10 = multibroadcast[out_lens={1, 5, 10},out_dyn_dims={}](@9) -> int32_type, {1, 5, 10}, {0, 10, 1}, target_id=0
@11 = multibroadcast[out_lens={1, 5, 10},out_dyn_dims={}](@10) -> int32_type, {1, 5, 10}, {0, 10, 1}, target_id=0
@12 = multibroadcast[out_lens={1, 5, 10},out_dyn_dims={}](@11) -> int32_type, {1, 5, 10}, {0, 10, 1}, target_id=0
@13 = pass(@12) -> int32_type, {1, 5, 10}, {0, 10, 1}, target_id=0
== y = @param:y -> int32_type, {1, 5, 1}, {5, 1, 1}, target_id=0
x = @param:x -> int32_type, {5, 10}, {10, 1}, target_id=0
@2 = multibroadcast[out_lens={1, 5, 10},out_dyn_dims={}](x) -> int32_type, {1, 5, 10}, {0, 10, 1}, target_id=0
@3 = multibroadcast[out_lens={1, 5, 10},out_dyn_dims={}](y) -> int32_type, {1, 5, 10}, {5, 1, 0}, target_id=0
@4 = add(@2,@3) -> int32_type, {1, 5, 10}, {0, 10, 1}, target_id=0
@5 = pass(@4) -> int32_type, {1, 5, 10}, {0, 10, 1}, target_id=0
]
[ FAILED ] simplify_inner_broadcast_no_common_axis: Test failure
[==========] 1 tests ran
[ FAILED ] 1 tests failed
[ FAILED ] simplify_inner_broadcast_no_common_axis
There was a problem hiding this comment.
Those multi broadcasts should get cleaned up by find_nop_reshaper later
There was a problem hiding this comment.
Also where all those broadcasts are being added. Looks to me that find_inner_broadcast would only add one multibroadcast after add
umangyadav
reviewed
Oct 11, 2023
inside simplify_reshapes, it is only adding unsqueeze if it is not scalar. Is it still an issue ? |
This is the behavior of unsqueeze on a scalar: Looks like it completely ignores the extra axes and instead converts from scalar to literal with stride 1. |
umangyadav
approved these changes
Oct 13, 2023
pfultz2
reviewed
Oct 13, 2023
| } | ||
| } | ||
| // if no common broadcast axis, transformation is not useful | ||
| if(std::find_if(common_axis.begin(), common_axis.end(), [](auto num_common) { |
There was a problem hiding this comment.
You could use std::none_of instead.
pfultz2
approved these changes
Oct 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Includes optimizations to
find_inner_broadcastbecause otherwise a bunch of multibroadcasts were appearing after the transformation.TODO: