-
Notifications
You must be signed in to change notification settings - Fork 90
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
Resize onnx operator: Optimization for Compute and Space performance of its linear option. #3773
Conversation
Is this sort of dup of #3731? |
No. Orthogonal and a more fundamental change to Resize parsing. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3773 +/- ##
========================================
Coverage 92.28% 92.29%
========================================
Files 519 519
Lines 22216 22233 +17
========================================
+ Hits 20503 20520 +17
Misses 1713 1713 ☔ View full report in Codecov by Sentry. |
Thanks for the explanation. |
(Background: going beyond the issue, #2129, the Resize Op could use more optimization in its basic calculations, hence this PR). |
I think these code changes make a merge conflict with the code in #3731 though? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to address tidy warning but otherwise LGTM
This build is not recommended to merge 🔴 |
🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output |
{ | ||
MIGRAPHX_THROW("PARSE_RESIZE: Shape dimension " + std::to_string(n_bits) + " exceeds " + | ||
std::to_string(std::numeric_limits<std::size_t>::digits)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error checking shouldn't be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the error check that belongs its caller api, if at all. So this is an exception one would hit if the lens dimension is 64 deep.
auto lo = vvv_ind[entry->second][0][e_idx]; | ||
auto hi = vvv_ind[entry->second][1][e_idx]; | ||
for(size_t i = 0; i < permutations; i++) | ||
perm_blk[i][l_idx] = ((i & hi_cmp_bit) != 0) ? hi : lo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this supposed to do? There is no explanation here. Using a bitset like the previous version would be better.
Optimize the space overhead required for Linear Resize operation: it is now 4x smaller for its 2D images. There were very large data-structures, getting to be over 16 times the total input_pixels for a 4D tensor. And now it becomes 4x smaller in size, followed with fewer reduction steps. (Similar optimization for its compute overhead.)
A comparison of parsing
test/onnx/upsample_linear_test.onnx
:(Before)
Calculated resize-tensor size:
@4 = @literal{ ... } -> int32_type, {16, 1, 4, 4}, {16, 16, 4, 1}
With this PR:
Calculated resize-tensor size:
@2 = @literal{ ... } -> int32_type, {4, 1, 4, 4}, {16, 16, 4, 1}