[Caffe2] MIOpen bug fixes and performance enhancements#11766
[Caffe2] MIOpen bug fixes and performance enhancements#11766ashishfarmer wants to merge 11 commits intopytorch:masterfrom ashishfarmer:af/miopen_fixes
Conversation
|
@ashishfarmer Thanks! Do you have data regarding the performance increase? |
|
@bddppq Is there a way we can run op tests on this particular PR? thx |
|
@petrex @ashishfarmer yes you can trigger the tests here https://ci.pytorch.org/jenkins/job/caffe2-builds/job/py2-clang3.8-rocm1.7.1-ubuntu16.04-trigger-test/build?delay=0sec with "GIT_COMMIT" as "origin/pr/YOUR_PR_NUMBER/head" (in this case it's "origin/pr/11766/head"). Triggered here: https://ci.pytorch.org/jenkins/job/caffe2-builds/job/py2-clang3.8-rocm1.7.1-ubuntu16.04-trigger-test/13272/ |
bddppq
left a comment
There was a problem hiding this comment.
LG. Could you add a test case that could trigger the issue that you fixed?
| beta_(OperatorBase::GetSingleArgument<float>("beta", 0.0)), | ||
| do_backward_( | ||
| OperatorBase::GetSingleArgument<bool>("do_backward", true)), | ||
| OperatorBase::GetSingleArgument<bool>("do_backward", false)), |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
Also pytorch/caffe2/operators/hip/spatial_batch_norm_op_miopen.cc Lines 143 to 144 in 5392b12 and pytorch/caffe2/operators/hip/spatial_batch_norm_op_miopen.cc Lines 278 to 279 in 5392b12 |
|
And here needs to update miopen_input_dims_ |
|
The case that is fixed in this PR (global pool gradient) is already covered by test_global_pooling in caffe2/python/operator_test/pooling_test.py |
|
@ashishfarmer lol then why was it not failing before? |
|
It was an intermittent failure before, that we got to the root cause of with this uninitialized variable issue. It is a very specific case (MIOpen engine + AveragePool + NCHW layout) when it will be triggered, and hypothesis would for the most part not encounter it. |
Removed these vectors |
Good catch! Thank you! Fixed this |
|
@bddppq - Added the fixes as per your review |
facebook-github-bot
left a comment
There was a problem hiding this comment.
bddppq has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: This PR contains changes for: 1. Performance enhancements for group conv using MIOpen 2. Performance enhancements by removing unnecessary computations while running pooling through MIOpen 3. Added check for bwdData comptutation while running MIOpen convGradient operator 4. Fix in MIOpen poolingGradient operator to compute window size for global pooling case 5. Minor code cleanup in MIOpen spatial batch norm operator Differential Revision: D9979050 Pulled By: bddppq fbshipit-source-id: fabc7a44a2f9ca0307d99564d1ce8fe1de9a6fbb
|
Merged to master in c7751f4 |
This PR contains changes for:
cc: @bddppq @petrex