[flang] [llvm] [mlir] [Flang]Fix for changed code at the end of AllocaIP. (PR #92430)
Mats Petersson via llvm-commits
llvm-commits at lists.llvm.org
Thu May 30 06:15:04 PDT 2024
https://github.com/Leporacanthicus updated https://github.com/llvm/llvm-project/pull/92430
>From 807c33831fe340893a1baa9d5ae79d2c33cbba9e Mon Sep 17 00:00:00 2001
From: Mats Petersson <mats.petersson at arm.com>
Date: Fri, 19 Apr 2024 18:00:58 +0100
Subject: [PATCH 1/3] Fix for changed code at the end of AllocaIP.
Some of the OpenMP code can change the instruction pointed at by
the insertion point. This leads to an assert in the compiler about
BB->getParent() and IP->getParent() not matching or something like
that.
The fix is to rebuild the insertionpoint from the block, rather
than use builder.restoreIP.
Also, move some of the alloca generation, rather than skipping back
and forth between insert points (and ensure all the allocas are done
before their users are created).
A simple test, mainly to ensure the minimal reproducer doesn't fail
to compile in the future is also added.
---
.../OpenMP/parallel-reduction-allocate.f90 | 23 +++++++++++++++++++
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | 11 ++++++---
.../OpenMP/OpenMPToLLVMIRTranslation.cpp | 20 ++++++++++------
3 files changed, 44 insertions(+), 10 deletions(-)
create mode 100644 flang/test/Lower/OpenMP/parallel-reduction-allocate.f90
diff --git a/flang/test/Lower/OpenMP/parallel-reduction-allocate.f90 b/flang/test/Lower/OpenMP/parallel-reduction-allocate.f90
new file mode 100644
index 0000000000000..fddce25ae22cc
--- /dev/null
+++ b/flang/test/Lower/OpenMP/parallel-reduction-allocate.f90
@@ -0,0 +1,23 @@
+!! The main point of this test is to check that the code compiles at all, so the
+!! checking is not very detailed. Not hitting an assert, crashing or otherwise failing
+!! to compile is the key point. Also, emitting llvm is required for this to happen.
+! RUN: %flang_fc1 -emit-llvm -fopenmp -o - %s 2>&1 | FileCheck %s
+subroutine proc
+ implicit none
+ real(8),allocatable :: F(:)
+ real(8),allocatable :: A(:)
+
+!$omp parallel private(A) reduction(+:F)
+ allocate(A(10))
+!$omp end parallel
+end subroutine proc
+
+!CHECK-LABEL: define void @proc_()
+!CHECK: call void
+!CHECK-SAME: @__kmpc_fork_call(ptr {{.*}}, i32 1, ptr @[[OMP_PAR:.*]], {{.*}})
+
+!CHECK: define internal void @[[OMP_PAR]]
+!CHECK: omp.par.region8:
+!CHECK-NEXT: call ptr @malloc
+!CHECK-SAME: i64 10
+
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index 9f0c07ef0da91..c0f4b5b066097 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -1391,7 +1391,8 @@ IRBuilder<>::InsertPoint OpenMPIRBuilder::createParallel(
// Change the location to the outer alloca insertion point to create and
// initialize the allocas we pass into the parallel region.
- Builder.restoreIP(OuterAllocaIP);
+ InsertPointTy NewOuter(OuterAllocaBlock, OuterAllocaBlock->begin());
+ Builder.restoreIP(NewOuter);
AllocaInst *TIDAddrAlloca = Builder.CreateAlloca(Int32, nullptr, "tid.addr");
AllocaInst *ZeroAddrAlloca =
Builder.CreateAlloca(Int32, nullptr, "zero.addr");
@@ -2135,7 +2136,8 @@ OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::createReductions(
// values.
unsigned NumReductions = ReductionInfos.size();
Type *RedArrayTy = ArrayType::get(Builder.getPtrTy(), NumReductions);
- Builder.restoreIP(AllocaIP);
+ Builder.SetInsertPoint(AllocaIP.getBlock()->getTerminator());
+ // Builder.restoreIP(AllocaIP);
Value *RedArray = Builder.CreateAlloca(RedArrayTy, nullptr, "red.array");
Builder.SetInsertPoint(InsertBlock, InsertBlock->end());
@@ -2536,7 +2538,10 @@ OpenMPIRBuilder::applyStaticWorkshareLoop(DebugLoc DL, CanonicalLoopInfo *CLI,
getOrCreateRuntimeFunction(M, omp::OMPRTL___kmpc_for_static_fini);
// Allocate space for computed loop bounds as expected by the "init" function.
- Builder.restoreIP(AllocaIP);
+
+ // Builder.restoreIP(AllocaIP);
+ Builder.SetInsertPoint(AllocaIP.getBlock()->getTerminator());
+
Type *I32Type = Type::getInt32Ty(M.getContext());
Value *PLastIter = Builder.CreateAlloca(I32Type, nullptr, "p.lastiter");
Value *PLowerBound = Builder.CreateAlloca(IVTy, nullptr, "p.lowerbound");
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index bfd7d65912bdb..b5f46b641f596 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -1194,6 +1194,17 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
MutableArrayRef<BlockArgument> reductionArgs =
opInst.getRegion().getArguments().take_back(
opInst.getNumReductionVars());
+
+ SmallVector<llvm::Value *> byRefVars;
+ if (isByRef) {
+ for (unsigned i = 0; i < opInst.getNumReductionVars(); ++i) {
+ // Allocate reduction variable (which is a pointer to the real reduciton
+ // variable allocated in the inlined region)
+ byRefVars.push_back(builder.CreateAlloca(
+ moduleTranslation.convertType(reductionDecls[i].getType())));
+ }
+ }
+
for (unsigned i = 0; i < opInst.getNumReductionVars(); ++i) {
SmallVector<llvm::Value *> phis;
@@ -1206,18 +1217,13 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
assert(phis.size() == 1 &&
"expected one value to be yielded from the "
"reduction neutral element declaration region");
- builder.restoreIP(allocaIP);
if (isByRef) {
- // Allocate reduction variable (which is a pointer to the real reduciton
- // variable allocated in the inlined region)
- llvm::Value *var = builder.CreateAlloca(
- moduleTranslation.convertType(reductionDecls[i].getType()));
// Store the result of the inlined region to the allocated reduction var
// ptr
- builder.CreateStore(phis[0], var);
+ builder.CreateStore(phis[0], byRefVars[i]);
- privateReductionVariables.push_back(var);
+ privateReductionVariables.push_back(byRefVars[i]);
moduleTranslation.mapValue(reductionArgs[i], phis[0]);
reductionVariableMap.try_emplace(opInst.getReductionVars()[i], phis[0]);
} else {
>From 0f61c9d3aed5b93b575bb72dde131912e278341a Mon Sep 17 00:00:00 2001
From: Mats Petersson <mats.petersson at arm.com>
Date: Wed, 29 May 2024 17:41:10 +0100
Subject: [PATCH 2/3] Make it work for both MLIR and Flang tests
---
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | 5 +----
.../LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | 9 ++++++++-
.../Target/LLVMIR/openmp-parallel-reduction-cleanup.mlir | 4 ++--
mlir/test/Target/LLVMIR/openmp-reduction-init-arg.mlir | 8 +++++---
4 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index c0f4b5b066097..7224e7ff64931 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -2137,10 +2137,9 @@ OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::createReductions(
unsigned NumReductions = ReductionInfos.size();
Type *RedArrayTy = ArrayType::get(Builder.getPtrTy(), NumReductions);
Builder.SetInsertPoint(AllocaIP.getBlock()->getTerminator());
- // Builder.restoreIP(AllocaIP);
Value *RedArray = Builder.CreateAlloca(RedArrayTy, nullptr, "red.array");
- Builder.SetInsertPoint(InsertBlock, InsertBlock->end());
+ Builder.SetInsertPoint(InsertBlock, InsertBlock->begin());
for (auto En : enumerate(ReductionInfos)) {
unsigned Index = En.index();
@@ -2538,8 +2537,6 @@ OpenMPIRBuilder::applyStaticWorkshareLoop(DebugLoc DL, CanonicalLoopInfo *CLI,
getOrCreateRuntimeFunction(M, omp::OMPRTL___kmpc_for_static_fini);
// Allocate space for computed loop bounds as expected by the "init" function.
-
- // Builder.restoreIP(AllocaIP);
Builder.SetInsertPoint(AllocaIP.getBlock()->getTerminator());
Type *I32Type = Type::getInt32Ty(M.getContext());
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index b5f46b641f596..bdc995c8be350 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -1195,6 +1195,7 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
opInst.getRegion().getArguments().take_back(
opInst.getNumReductionVars());
+ llvm::BasicBlock *initBlock = nullptr;
SmallVector<llvm::Value *> byRefVars;
if (isByRef) {
for (unsigned i = 0; i < opInst.getNumReductionVars(); ++i) {
@@ -1203,6 +1204,9 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
byRefVars.push_back(builder.CreateAlloca(
moduleTranslation.convertType(reductionDecls[i].getType())));
}
+
+ initBlock = splitBB(builder, true, "omp.reduction.init");
+ allocaIP = InsertPointTy(allocaIP.getBlock(), allocaIP.getBlock()->end());
}
for (unsigned i = 0; i < opInst.getNumReductionVars(); ++i) {
@@ -1217,7 +1221,10 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
assert(phis.size() == 1 &&
"expected one value to be yielded from the "
"reduction neutral element declaration region");
-
+ if (initBlock)
+ builder.SetInsertPoint(initBlock->getTerminator());
+ else
+ builder.restoreIP(allocaIP);
if (isByRef) {
// Store the result of the inlined region to the allocated reduction var
// ptr
diff --git a/mlir/test/Target/LLVMIR/openmp-parallel-reduction-cleanup.mlir b/mlir/test/Target/LLVMIR/openmp-parallel-reduction-cleanup.mlir
index b7f71f438e56b..34f688e471a32 100644
--- a/mlir/test/Target/LLVMIR/openmp-parallel-reduction-cleanup.mlir
+++ b/mlir/test/Target/LLVMIR/openmp-parallel-reduction-cleanup.mlir
@@ -55,11 +55,11 @@
// Private reduction variable and its initialization.
// CHECK: %tid.addr.local = alloca i32
-// CHECK: %[[MALLOC_I:.+]] = call ptr @malloc(i64 4)
// CHECK: %[[PRIV_PTR_I:.+]] = alloca ptr
+// CHECK: %[[PRIV_PTR_J:.+]] = alloca ptr
+// CHECK: %[[MALLOC_I:.+]] = call ptr @malloc(i64 4)
// CHECK: store ptr %[[MALLOC_I]], ptr %[[PRIV_PTR_I]]
// CHECK: %[[MALLOC_J:.+]] = call ptr @malloc(i64 4)
-// CHECK: %[[PRIV_PTR_J:.+]] = alloca ptr
// CHECK: store ptr %[[MALLOC_J]], ptr %[[PRIV_PTR_J]]
// Call to the reduction function.
diff --git a/mlir/test/Target/LLVMIR/openmp-reduction-init-arg.mlir b/mlir/test/Target/LLVMIR/openmp-reduction-init-arg.mlir
index 5dd31c425566c..6e0b0086e528f 100644
--- a/mlir/test/Target/LLVMIR/openmp-reduction-init-arg.mlir
+++ b/mlir/test/Target/LLVMIR/openmp-reduction-init-arg.mlir
@@ -59,13 +59,15 @@ module {
// CHECK: %[[VAL_17:.*]] = load i32, ptr %[[VAL_18:.*]], align 4
// CHECK: store i32 %[[VAL_17]], ptr %[[VAL_16]], align 4
// CHECK: %[[VAL_19:.*]] = load i32, ptr %[[VAL_16]], align 4
-// CHECK: %[[VAL_20:.*]] = load { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]] }, ptr %[[VAL_13]], align 8
// CHECK: %[[VAL_21:.*]] = alloca ptr, align 8
+// CHECK: %[[VAL_23:.*]] = alloca ptr, align 8
+// CHECK: %[[VAL_20:.*]] = load { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]] }, ptr %[[VAL_13]], align 8
+// CHECK: %[[VAL_24:.*]] = alloca [2 x ptr], align 8
+// CHECK: br label %[[INIT_LABEL:.*]]
+// CHECK: [[INIT_LABEL]]:
// CHECK: store ptr %[[VAL_13]], ptr %[[VAL_21]], align 8
// CHECK: %[[VAL_22:.*]] = load { ptr, i64, i32, i8, i8, i8, i8, [1 x [3 x i64]] }, ptr %[[VAL_15]], align 8
-// CHECK: %[[VAL_23:.*]] = alloca ptr, align 8
// CHECK: store ptr %[[VAL_15]], ptr %[[VAL_23]], align 8
-// CHECK: %[[VAL_24:.*]] = alloca [2 x ptr], align 8
// CHECK: br label %[[VAL_25:.*]]
// CHECK: omp.par.region: ; preds = %[[VAL_26:.*]]
// CHECK: br label %[[VAL_27:.*]]
>From b0875c4e11263e69169c8f51694cefab4c1126f9 Mon Sep 17 00:00:00 2001
From: Mats Petersson <mats.petersson at arm.com>
Date: Thu, 30 May 2024 13:54:13 +0100
Subject: [PATCH 3/3] Review fixes
---
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | 4 ++--
.../Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | 15 +++++++--------
2 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index 7224e7ff64931..004e039418c7e 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -2139,7 +2139,7 @@ OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::createReductions(
Builder.SetInsertPoint(AllocaIP.getBlock()->getTerminator());
Value *RedArray = Builder.CreateAlloca(RedArrayTy, nullptr, "red.array");
- Builder.SetInsertPoint(InsertBlock, InsertBlock->begin());
+ Builder.SetInsertPoint(InsertBlock, InsertBlock->end());
for (auto En : enumerate(ReductionInfos)) {
unsigned Index = En.index();
@@ -3100,7 +3100,7 @@ OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::applyDynamicWorkshareLoop(
FunctionCallee DynamicNext = getKmpcForDynamicNextForType(IVTy, M, *this);
// Allocate space for computed loop bounds as expected by the "init" function.
- Builder.restoreIP(AllocaIP);
+ Builder.SetInsertPoint(AllocaIP.getBlock()->getTerminator());
Type *I32Type = Type::getInt32Ty(M.getContext());
Value *PLastIter = Builder.CreateAlloca(I32Type, nullptr, "p.lastiter");
Value *PLowerBound = Builder.CreateAlloca(IVTy, nullptr, "p.lowerbound");
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index bdc995c8be350..e790a150c16ed 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -813,7 +813,7 @@ static void allocByValReductionVars(
SmallVectorImpl<llvm::Value *> &privateReductionVariables,
DenseMap<Value, llvm::Value *> &reductionVariableMap) {
llvm::IRBuilderBase::InsertPointGuard guard(builder);
- builder.restoreIP(allocaIP);
+ builder.SetInsertPoint(allocaIP.getBlock()->getTerminator());
auto args =
loop.getRegion().getArguments().take_back(loop.getNumReductionVars());
@@ -1195,7 +1195,10 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
opInst.getRegion().getArguments().take_back(
opInst.getNumReductionVars());
- llvm::BasicBlock *initBlock = nullptr;
+ llvm::BasicBlock *initBlock = splitBB(builder, true, "omp.reduction.init");
+ allocaIP =
+ InsertPointTy(allocaIP.getBlock(),
+ allocaIP.getBlock()->getTerminator()->getIterator());
SmallVector<llvm::Value *> byRefVars;
if (isByRef) {
for (unsigned i = 0; i < opInst.getNumReductionVars(); ++i) {
@@ -1205,8 +1208,6 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
moduleTranslation.convertType(reductionDecls[i].getType())));
}
- initBlock = splitBB(builder, true, "omp.reduction.init");
- allocaIP = InsertPointTy(allocaIP.getBlock(), allocaIP.getBlock()->end());
}
for (unsigned i = 0; i < opInst.getNumReductionVars(); ++i) {
@@ -1221,10 +1222,8 @@ convertOmpParallel(omp::ParallelOp opInst, llvm::IRBuilderBase &builder,
assert(phis.size() == 1 &&
"expected one value to be yielded from the "
"reduction neutral element declaration region");
- if (initBlock)
- builder.SetInsertPoint(initBlock->getTerminator());
- else
- builder.restoreIP(allocaIP);
+ builder.SetInsertPoint(initBlock->getTerminator());
+
if (isByRef) {
// Store the result of the inlined region to the allocated reduction var
// ptr
More information about the llvm-commits
mailing list