[offload] Remove bad assert in StaticLoopChunker::Distribute #132705
Conversation
When building with asserts enabled, this can actually cause strange miscompilations because an incorrect llvm.assume is generated at the point of the assertion.
|
@llvm/pr-subscribers-offload Author: None (macurtis-amd) ChangesWhen building with asserts enabled, this can actually cause strange miscompilations because an incorrect llvm.assume is generated at the point of the assertion. Full diff: https://github.com/llvm/llvm-project/pull/132705.diff 2 Files Affected:
diff --git a/offload/DeviceRTL/src/Workshare.cpp b/offload/DeviceRTL/src/Workshare.cpp
index de4ed2e2102a6..861b9ca371ccd 100644
--- a/offload/DeviceRTL/src/Workshare.cpp
+++ b/offload/DeviceRTL/src/Workshare.cpp
@@ -820,7 +820,6 @@ template <typename Ty> class StaticLoopChunker {
Ty ThreadChunk = 0;
Ty NumThreads = 1;
Ty TId = 0;
- ASSERT(TId == mapping::getThreadIdInBlock(), "Bad thread id");
// All teams need to participate.
Ty NumBlocks = mapping::getNumberOfBlocksInKernel();
diff --git a/offload/test/offloading/fortran/target-teams-dist-nest-par.f90 b/offload/test/offloading/fortran/target-teams-dist-nest-par.f90
new file mode 100644
index 0000000000000..dfde1b98f3c86
--- /dev/null
+++ b/offload/test/offloading/fortran/target-teams-dist-nest-par.f90
@@ -0,0 +1,26 @@
+! REQUIRES: flang, amdgpu
+
+! RUN: %libomptarget-compile-fortran-generic
+! RUN: %libomptarget-run-generic 2>&1 | %fcheck-generic
+program main
+ integer :: array(10) = 0
+ integer :: x, y, z
+ !$omp target
+ !$omp teams distribute private(x, y)
+ OuterLoopOne: do x=1,1
+ array(2) = 42
+ OuterLoopTwo: do y=1,1
+ !$omp parallel do private(z)
+ InnerLoopOne: do z=1,10
+ array(z) = 20
+ enddo InnerLoopOne
+ !$omp end parallel do
+ enddo OuterLoopTwo
+ enddo OuterLoopOne
+ !$omp end teams distribute
+ !$omp end target
+ ! Expected to print all 20's
+ print *, array
+end program main
+
+! CHECK: 20 20 20 20 20 20 20 20 20 20
|
| @@ -820,7 +820,6 @@ template <typename Ty> class StaticLoopChunker { | |||
| Ty ThreadChunk = 0; | |||
| Ty NumThreads = 1; | |||
| Ty TId = 0; | |||
| ASSERT(TId == mapping::getThreadIdInBlock(), "Bad thread id"); | |||
There was a problem hiding this comment.
The three lines of code above assume this should be the case.
There was a problem hiding this comment.
So this code is only ever expected to run with thread zero? That seems wrong because I thought even in generic mode we used a warp at the end of the block or something.
There was a problem hiding this comment.
Yes, but in the last warp we only use one thread instead of all of them.
There was a problem hiding this comment.
But this asserts that only thread zero will ever execute here.
There was a problem hiding this comment.
I think we need @DominikAdamski to provide some insight into this code. I can only comment based on empirical experience, with the assumption that flang is generating correct calls into the runtime.
FWIW, this code (i.e. __kmpc_distribute_static_loop*) does not seem to be used by clang.
…2705) When building with asserts enabled, this can actually cause strange miscompilations because an incorrect llvm.assume is generated at the point of the assertion.
When building with asserts enabled, this can actually cause strange miscompilations because an incorrect llvm.assume is generated at the point of the assertion.