[OpenMP][LLVM] Update alloca IP after PrivCB in OMPIRBUIlder
#93920
Conversation
llvmbot
added
flang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-flang-openmp Author: Kareem Ergawy (ergawy) ChangesFixes a crash uncovered by pr77666.f90 in the test suite. In particular, whenever Full diff: https://github.com/llvm/llvm-project/pull/93920.diff 2 Files Affected:
diff --git a/flang/test/Lower/OpenMP/delayed-privatization-lower-allocatable-to-llvm.f90 b/flang/test/Lower/OpenMP/delayed-privatization-lower-allocatable-to-llvm.f90
new file mode 100644
index 0000000000000..0363494c59ecf
--- /dev/null
+++ b/flang/test/Lower/OpenMP/delayed-privatization-lower-allocatable-to-llvm.f90
@@ -0,0 +1,20 @@
+! RUN: %flang -S -emit-llvm -fopenmp -mmlir --openmp-enable-delayed-privatization \
+! RUN: -o - %s 2>&1 | FileCheck %s
+
+subroutine foo(x)
+ integer, allocatable :: x, y
+!$omp parallel private(x, y)
+ x = y
+!$omp end parallel
+end
+
+! CHECK-LABEL: define void @foo_
+! CHECK: ret void
+! CHECK-NEXT: }
+
+! CHECK-LABEL: define internal void @foo_..omp_par
+! CHECK-DAG: call ptr @malloc
+! CHECK-DAG: call ptr @malloc
+! CHECK-DAG: call void @free
+! CHECK-DAG: call void @free
+! CHECK: }
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index cb4de9c8876dc..eab41eb8a35b2 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -1583,6 +1583,9 @@ IRBuilder<>::InsertPoint OpenMPIRBuilder::createParallel(
} else {
Builder.restoreIP(
PrivCB(InnerAllocaIP, Builder.saveIP(), V, *Inner, ReplacementValue));
+ InnerAllocaIP = {InnerAllocaIP.getPoint()->getParent(),
+ InnerAllocaIP.getPoint()};
+
assert(ReplacementValue &&
"Expected copy/create callback to set replacement value!");
if (ReplacementValue == &V)
|
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Kareem Ergawy (ergawy) ChangesFixes a crash uncovered by pr77666.f90 in the test suite. In particular, whenever Full diff: https://github.com/llvm/llvm-project/pull/93920.diff 2 Files Affected:
diff --git a/flang/test/Lower/OpenMP/delayed-privatization-lower-allocatable-to-llvm.f90 b/flang/test/Lower/OpenMP/delayed-privatization-lower-allocatable-to-llvm.f90
new file mode 100644
index 0000000000000..0363494c59ecf
--- /dev/null
+++ b/flang/test/Lower/OpenMP/delayed-privatization-lower-allocatable-to-llvm.f90
@@ -0,0 +1,20 @@
+! RUN: %flang -S -emit-llvm -fopenmp -mmlir --openmp-enable-delayed-privatization \
+! RUN: -o - %s 2>&1 | FileCheck %s
+
+subroutine foo(x)
+ integer, allocatable :: x, y
+!$omp parallel private(x, y)
+ x = y
+!$omp end parallel
+end
+
+! CHECK-LABEL: define void @foo_
+! CHECK: ret void
+! CHECK-NEXT: }
+
+! CHECK-LABEL: define internal void @foo_..omp_par
+! CHECK-DAG: call ptr @malloc
+! CHECK-DAG: call ptr @malloc
+! CHECK-DAG: call void @free
+! CHECK-DAG: call void @free
+! CHECK: }
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index cb4de9c8876dc..eab41eb8a35b2 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -1583,6 +1583,9 @@ IRBuilder<>::InsertPoint OpenMPIRBuilder::createParallel(
} else {
Builder.restoreIP(
PrivCB(InnerAllocaIP, Builder.saveIP(), V, *Inner, ReplacementValue));
+ InnerAllocaIP = {InnerAllocaIP.getPoint()->getParent(),
+ InnerAllocaIP.getPoint()};
+
assert(ReplacementValue &&
"Expected copy/create callback to set replacement value!");
if (ReplacementValue == &V)
|
Fixes a crash uncovered by [pr77666.f90](https://github.com/llvm/llvm-test-suite/blob/main/Fortran/gfortran/regression/gomp/pr77666.f90) in the test suite. In particular, whenever `PrivCB` (the callback responsible for generating privatizaiton logic for an OMP variable) generates a multi-block privatization region, the insertion point diverges: the BB component of the IP can become a different BB from the parent block of the instruction iterator component of the IP. This PR updates the IP to make sure that the BB is the parent block of the instruction iterator.
ergawy
force-pushed
the
delayed_privatization_25
branch
from
e4dfb33 to
926cf8d
Compare
ergawy
changed the title
PrivCB in OMPIRBUIlderPrivCB in OMPIRBUIlder
|
Probably similar to #92430 |
| @@ -1583,6 +1583,9 @@ IRBuilder<>::InsertPoint OpenMPIRBuilder::createParallel( | |||
| } else { | |||
| Builder.restoreIP( | |||
| PrivCB(InnerAllocaIP, Builder.saveIP(), V, *Inner, ReplacementValue)); | |||
| InnerAllocaIP = {InnerAllocaIP.getPoint()->getParent(), | |||
There was a problem hiding this comment.
So, the problem leading to this is that we've split the Alloca block [because the variable is allocatable], and the actual insert point is now the branch of the second block.
I don't think we want the new AllocaIP to be in that second block, in case someone adds more alloca ops - they should go into the first block.
My PR, that Kiran mentioned, has several of this kind of fixes [I will check if it fixes this case too,]. Whenever possible, I try to make the new location at the terminator of the original AllocaIP block.
There was a problem hiding this comment.
My PR, that Kiran mentioned, has several of this kind of fixes [I will check if it fixes this case too,]. Whenever possible, I try to make the new location at the terminator of the original AllocaIP block.
Thanks for looking at this @Leporacanthicus. I tried your PR with a reproducing example and so far it does not handle this case. If you like, you can take the test I added in this PR and add it to yours and handle that case in your PR as well. Either way is fine with me.
There was a problem hiding this comment.
@kiranchandramohan @Leporacanthicus based on the above comment, and since neither PRs solves the "1 BB for all allocas" problem and since #92430 does not solve the crash handled by this PR, I think there is not conflict between this PR and #92430. I think we can proceed with both. WDYT?
There was a problem hiding this comment.
I'm hitting further problems (in the real code that we're trying to make work), so I'm OK with this going in if it is helping you get the job done. I personally would prefer it going into the oriiginal allocaIP BB - I'm well aware that not ALL things go in the original BB, but we don't need to make it worse... :)
Also, if all places that need this fix use code like builder.SetInsertPoint(allocaIP.getBlock()->getTerminator()); or perhaps:
allocaIP =
InsertPointTy(allocaIP.getBlock(),
allocaIP.getBlock()->getTerminator()->getIterator());
then it makes it a little easier to find ALL such places, and not have to find half a dozen different variants that do slightly different things.
There was a problem hiding this comment.
allocaIP = InsertPointTy(allocaIP.getBlock(), allocaIP.getBlock()->getTerminator()->getIterator());
I can do that. I actually tried that locally and did not commit it since it did not make much difference in terms of generated IR. However, it makes sense to commit it given the point you mentioned about consistency.
Fixes a crash uncovered by pr77666.f90 in the test suite when delayed privatization is enabled by default.
In particular, whenever
PrivCB(the callback responsible for generating privatizaiton logic for an OMP variable) generates a multi-block privatization region, the insertion point diverges: the BB component of the IP can become a different BB from the parent block of the instruction iterator component of the IP. This PR updates the IP to make sure that the BB is the parent block of the instruction iterator.