[ROCm] Fused convolution+bias+activation #9666
Conversation
| }; | ||
| std::thread threads[4]; | ||
| for(int i=0; i<4; i++) | ||
| threads[i] = std::thread(executor, i); |
There was a problem hiding this comment.
What is the motivation for the multi thread test? All threads run the same module, right?
There was a problem hiding this comment.
This would potentially flag race conditions due to MIOpen (or CuDNN) executing identical fused operations in different threads.
There was a problem hiding this comment.
The race condition would need to make Run(...) return false for this test to catch it, which may or may not happen. Wouldn't running under ThreadSanetizer give you better chances of finding such issues? Also, this test class doesn't seem the right place to test MIOpen or cuDNN for thread safety, in my opinion.
There was a problem hiding this comment.
Did you reach an agreement about this with @thomasjoerg?
It looks like the test is still there.
There was a problem hiding this comment.
The test is still there, but it was changed to use RunAndCompare instead of Run, so it would catch mismatches. I don't see any other places where backend implementations of fused convolutions are tested or could be tested for concurrency issues.
As it is, worst thing that might happen is that there might be an issue but the test won't catch it. I can certainly remove the test, but I'm not sure how dropping the test would improve matters.
There was a problem hiding this comment.
Tests need to be maintained and in its current form this test will be hard to maintain. When this test breaks, the person looking at the breakage needs to right information to understand the issue.
- A code comment should explain the motivation for this test, because it is different from the non-threaded tests around it.
- Please explain where the race condition would occur. I'm not sure here - the threads don't seem to share anything and don't communicate.
- This test does not test XLA's conv rewriter, but MIOpen / cuDNN, right? This is not obvious and deserves a comment too. Would XLA actually call convs on multiple threads (inter-op parallelism)?
There was a problem hiding this comment.
The threads don't share anything and they don't communicate, but, at least in the MIOpen case, stream executor will cache and reuse the same "fusion plan" handle for operations with identical parameters (https://github.com/openxla/xla/blob/main/xla/stream_executor/rocm/rocm_dnn.cc#L560), and potentially the same handle may be used in parallel in different threads. CuDNN may be doing something similar at some level.
Since this PR is almost 2 months old by now, rather than argue the point further, I'm going to drop the test. If race conditions are really possible, someone will have fun tracking them down the hard way.
ekuznetsov139
force-pushed
the
ci_fused_conv
branch
from
6123d58 to
3e74da9
Compare
ekuznetsov139
force-pushed
the
ci_fused_conv
branch
from
3e74da9 to
03ea211
Compare
ekuznetsov139
force-pushed
the
ci_fused_conv
branch
from
03ea211 to
6b38aed
Compare
ekuznetsov139
force-pushed
the
ci_fused_conv
branch
from
6b38aed to
630debf
Compare
ekuznetsov139
force-pushed
the
ci_fused_conv
branch
from
630debf to
8fd3748
Compare
|
Bump |
|
Hi @tdanyluk , can you look into this once? Thanks. |
ekuznetsov139
force-pushed
the
ci_fused_conv
branch
from
8fd3748 to
b56d07d
Compare
|
Hi @thomasjoerg , can you please look into this ? Thanks. |
ekuznetsov139
force-pushed
the
ci_fused_conv
branch
from
7cb82b9 to
4fb5629
Compare
ekuznetsov139
force-pushed
the
ci_fused_conv
branch
from
4fb5629 to
8792f89
Compare
Imported from GitHub PR #9666 This PR enables MIOpen convolution+bias+activation fusion for ROCm and updates fusion unit tests accordingly. Copybara import of the project: -- bd5be49 by Eugene Kuznetsov <[email protected]>: Switch to NHWC for ROCm and F16 -- 8792f89 by Eugene Kuznetsov <[email protected]>: Fused convolution+bias+activation Merging this change closes #9666 FUTURE_COPYBARA_INTEGRATE_REVIEW=#9666 from ROCm:ci_fused_conv 8792f89 PiperOrigin-RevId: 633859923
Imported from GitHub PR openxla/xla#9666 This PR enables MIOpen convolution+bias+activation fusion for ROCm and updates fusion unit tests accordingly. Copybara import of the project: -- bd5be494abe6621dfd2c4ebcca4f8992077d8a89 by Eugene Kuznetsov <[email protected]>: Switch to NHWC for ROCm and F16 -- 8792f892770aba54e38a5352d9bec1d003e341d3 by Eugene Kuznetsov <[email protected]>: Fused convolution+bias+activation Merging this change closes #9666 FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#9666 from ROCm:ci_fused_conv 8792f892770aba54e38a5352d9bec1d003e341d3 PiperOrigin-RevId: 633859923
Imported from GitHub PR #9666 This PR enables MIOpen convolution+bias+activation fusion for ROCm and updates fusion unit tests accordingly. Copybara import of the project: -- bd5be49 by Eugene Kuznetsov <[email protected]>: Switch to NHWC for ROCm and F16 -- 8792f89 by Eugene Kuznetsov <[email protected]>: Fused convolution+bias+activation Merging this change closes #9666 FUTURE_COPYBARA_INTEGRATE_REVIEW=#9666 from ROCm:ci_fused_conv 8792f89 PiperOrigin-RevId: 633859923
Imported from GitHub PR #9666 This PR enables MIOpen convolution+bias+activation fusion for ROCm and updates fusion unit tests accordingly. Copybara import of the project: -- bd5be49 by Eugene Kuznetsov <[email protected]>: Switch to NHWC for ROCm and F16 -- 8792f89 by Eugene Kuznetsov <[email protected]>: Fused convolution+bias+activation Merging this change closes #9666 FUTURE_COPYBARA_INTEGRATE_REVIEW=#9666 from ROCm:ci_fused_conv 8792f89 PiperOrigin-RevId: 633859923
Imported from GitHub PR openxla/xla#9666 This PR enables MIOpen convolution+bias+activation fusion for ROCm and updates fusion unit tests accordingly. Copybara import of the project: -- bd5be494abe6621dfd2c4ebcca4f8992077d8a89 by Eugene Kuznetsov <[email protected]>: Switch to NHWC for ROCm and F16 -- 8792f892770aba54e38a5352d9bec1d003e341d3 by Eugene Kuznetsov <[email protected]>: Fused convolution+bias+activation Merging this change closes #9666 FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#9666 from ROCm:ci_fused_conv 8792f892770aba54e38a5352d9bec1d003e341d3 PiperOrigin-RevId: 633859923
Imported from GitHub PR #9666 This PR enables MIOpen convolution+bias+activation fusion for ROCm and updates fusion unit tests accordingly. Copybara import of the project: -- bd5be49 by Eugene Kuznetsov <[email protected]>: Switch to NHWC for ROCm and F16 -- 8792f89 by Eugene Kuznetsov <[email protected]>: Fused convolution+bias+activation Merging this change closes #9666 FUTURE_COPYBARA_INTEGRATE_REVIEW=#9666 from ROCm:ci_fused_conv 8792f89 PiperOrigin-RevId: 633859923
Imported from GitHub PR openxla/xla#9666 This PR enables MIOpen convolution+bias+activation fusion for ROCm and updates fusion unit tests accordingly. Copybara import of the project: -- bd5be494abe6621dfd2c4ebcca4f8992077d8a89 by Eugene Kuznetsov <[email protected]>: Switch to NHWC for ROCm and F16 -- 8792f892770aba54e38a5352d9bec1d003e341d3 by Eugene Kuznetsov <[email protected]>: Fused convolution+bias+activation Merging this change closes #9666 FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#9666 from ROCm:ci_fused_conv 8792f892770aba54e38a5352d9bec1d003e341d3 PiperOrigin-RevId: 633859923
Imported from GitHub PR openxla/xla#9666 This PR enables MIOpen convolution+bias+activation fusion for ROCm and updates fusion unit tests accordingly. Copybara import of the project: -- bd5be494abe6621dfd2c4ebcca4f8992077d8a89 by Eugene Kuznetsov <[email protected]>: Switch to NHWC for ROCm and F16 -- 8792f892770aba54e38a5352d9bec1d003e341d3 by Eugene Kuznetsov <[email protected]>: Fused convolution+bias+activation Merging this change closes #9666 PiperOrigin-RevId: 633923105
…strategy scope. In some cases `v._distribute_strategy` can be None, which was causing an AttributeError when trying to check if the `extended` field matched the expected value. FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#9666 from ROCm:ci_fused_conv 8792f892770aba54e38a5352d9bec1d003e341d3 PiperOrigin-RevId: 633574887
The OneDNN matmul rewriter starts a thread pool if one wasn't provided as a compile option, spawning many threads per compilation. If we pass a preexisting thread pool, we can save the cost of creating and tearing down threads. FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#9666 from ROCm:ci_fused_conv 8792f892770aba54e38a5352d9bec1d003e341d3 PiperOrigin-RevId: 633926908
This PR enables MIOpen convolution+bias+activation fusion for ROCm and updates fusion unit tests accordingly.