[Mlir-commits] [llvm] [mlir] [flang][OMPIRbuilder] Set debug loc on terminator created by splitBB. (PR #125897)
Abid Qadeer
llvmlistbot at llvm.org
Wed Feb 5 09:51:49 PST 2025
https://github.com/abidh created https://github.com/llvm/llvm-project/pull/125897
Fixes #125088.
When splitBB is called with createBranch=true, it creates a branch instruction in the old block. But no debug loc is set on that branch instruction. If that is used as InsertPoint in the restoreIP, it has the potential to set the current debug location to null and subsequent instruction will come out without a debug location. This caused the verification check to fail as shown in the bug report.
This PR changes splitBB and spliceBB function to also take a debugLoc parameter which can be used to set the debug location of the branch instruction.
>From 54a890913229ac7c1eeb645cec57e7e5786f3daf Mon Sep 17 00:00:00 2001
From: Abid Qadeer <haqadeer at amd.com>
Date: Fri, 31 Jan 2025 12:41:54 +0000
Subject: [PATCH] [flang][OpenMP] Set debug loc on terminator created by
splitBB.
Fixes #125088.
When splitBB is called with createBranch=true, it creates a branch
instruction in the old block. But no debug loc is set on that branch
instruction. If that is used as InsertPoint in the restoreIP, it has
the potential to set the current debug location to null and subsequent
instruction will come out without a debug location. This caused the
verification check to fail as shown in the bug report.
This PR changes splitBB and spliceBB function to also take a debugLoc
parameter which can be used to set the debug location of the branch
instruction.
---
.../llvm/Frontend/OpenMP/OMPIRBuilder.h | 12 +++---
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | 20 +++++-----
.../Frontend/OpenMPIRBuilderTest.cpp | 16 ++++++++
.../OpenMP/OpenMPToLLVMIRTranslation.cpp | 3 +-
.../Target/LLVMIR/omptarget-debug-nowait.mlir | 40 +++++++++++++++++++
5 files changed, 76 insertions(+), 15 deletions(-)
create mode 100644 mlir/test/Target/LLVMIR/omptarget-debug-nowait.mlir
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h b/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
index 9802cbe8b7b9438..d25077cae63e42b 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
+++ b/llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
@@ -40,9 +40,10 @@ class OpenMPIRBuilder;
/// not have any PHINodes. If \p CreateBranch is true, a branch instruction to
/// \p New will be added such that there is no semantic change. Otherwise, the
/// \p IP insert block remains degenerate and it is up to the caller to insert a
-/// terminator.
-void spliceBB(IRBuilderBase::InsertPoint IP, BasicBlock *New,
- bool CreateBranch);
+/// terminator. \p DL is used as the debug location for the branch instruction
+/// if one is created.
+void spliceBB(IRBuilderBase::InsertPoint IP, BasicBlock *New, bool CreateBranch,
+ DebugLoc DL);
/// Splice a BasicBlock at an IRBuilder's current insertion point. Its new
/// insert location will stick to after the instruction before the insertion
@@ -58,9 +59,10 @@ void spliceBB(IRBuilder<> &Builder, BasicBlock *New, bool CreateBranch);
/// is true, a branch to the new successor will new created such that
/// semantically there is no change; otherwise the block of the insertion point
/// remains degenerate and it is the caller's responsibility to insert a
-/// terminator. Returns the new successor block.
+/// terminator. \p DL is used as the debug location for the branch instruction
+/// if one is created. Returns the new successor block.
BasicBlock *splitBB(IRBuilderBase::InsertPoint IP, bool CreateBranch,
- llvm::Twine Name = {});
+ DebugLoc DL, llvm::Twine Name = {});
/// Split a BasicBlock at \p Builder's insertion point, even if the block is
/// degenerate (missing the terminator). Its new insert location will stick to
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index 695b15ac31f380e..490a012f696034a 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -313,7 +313,7 @@ static void redirectTo(BasicBlock *Source, BasicBlock *Target, DebugLoc DL) {
}
void llvm::spliceBB(IRBuilderBase::InsertPoint IP, BasicBlock *New,
- bool CreateBranch) {
+ bool CreateBranch, DebugLoc DL) {
assert(New->getFirstInsertionPt() == New->begin() &&
"Target BB must not have PHI nodes");
@@ -321,15 +321,17 @@ void llvm::spliceBB(IRBuilderBase::InsertPoint IP, BasicBlock *New,
BasicBlock *Old = IP.getBlock();
New->splice(New->begin(), Old, IP.getPoint(), Old->end());
- if (CreateBranch)
- BranchInst::Create(New, Old);
+ if (CreateBranch) {
+ auto *NewBr = BranchInst::Create(New, Old);
+ NewBr->setDebugLoc(DL);
+ }
}
void llvm::spliceBB(IRBuilder<> &Builder, BasicBlock *New, bool CreateBranch) {
DebugLoc DebugLoc = Builder.getCurrentDebugLocation();
BasicBlock *Old = Builder.GetInsertBlock();
- spliceBB(Builder.saveIP(), New, CreateBranch);
+ spliceBB(Builder.saveIP(), New, CreateBranch, DebugLoc);
if (CreateBranch)
Builder.SetInsertPoint(Old->getTerminator());
else
@@ -341,12 +343,12 @@ void llvm::spliceBB(IRBuilder<> &Builder, BasicBlock *New, bool CreateBranch) {
}
BasicBlock *llvm::splitBB(IRBuilderBase::InsertPoint IP, bool CreateBranch,
- llvm::Twine Name) {
+ DebugLoc DL, llvm::Twine Name) {
BasicBlock *Old = IP.getBlock();
BasicBlock *New = BasicBlock::Create(
Old->getContext(), Name.isTriviallyEmpty() ? Old->getName() : Name,
Old->getParent(), Old->getNextNode());
- spliceBB(IP, New, CreateBranch);
+ spliceBB(IP, New, CreateBranch, DL);
New->replaceSuccessorsPhiUsesWith(Old, New);
return New;
}
@@ -354,7 +356,7 @@ BasicBlock *llvm::splitBB(IRBuilderBase::InsertPoint IP, bool CreateBranch,
BasicBlock *llvm::splitBB(IRBuilderBase &Builder, bool CreateBranch,
llvm::Twine Name) {
DebugLoc DebugLoc = Builder.getCurrentDebugLocation();
- BasicBlock *New = splitBB(Builder.saveIP(), CreateBranch, Name);
+ BasicBlock *New = splitBB(Builder.saveIP(), CreateBranch, DebugLoc, Name);
if (CreateBranch)
Builder.SetInsertPoint(Builder.GetInsertBlock()->getTerminator());
else
@@ -368,7 +370,7 @@ BasicBlock *llvm::splitBB(IRBuilderBase &Builder, bool CreateBranch,
BasicBlock *llvm::splitBB(IRBuilder<> &Builder, bool CreateBranch,
llvm::Twine Name) {
DebugLoc DebugLoc = Builder.getCurrentDebugLocation();
- BasicBlock *New = splitBB(Builder.saveIP(), CreateBranch, Name);
+ BasicBlock *New = splitBB(Builder.saveIP(), CreateBranch, DebugLoc, Name);
if (CreateBranch)
Builder.SetInsertPoint(Builder.GetInsertBlock()->getTerminator());
else
@@ -5303,7 +5305,7 @@ void OpenMPIRBuilder::createIfVersion(CanonicalLoopInfo *CanonicalLoop,
Builder.CreateCondBr(IfCond, ThenBlock, /*ifFalse*/ ElseBlock);
InsertPointTy IP{BrInstr->getParent(), ++BrInstr->getIterator()};
// Then block contains branch to omp loop which needs to be vectorized
- spliceBB(IP, ThenBlock, false);
+ spliceBB(IP, ThenBlock, false, Builder.getCurrentDebugLocation());
ThenBlock->replaceSuccessorsPhiUsesWith(Head, ThenBlock);
Builder.SetInsertPoint(ElseBlock);
diff --git a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
index 9e0a338843d6603..83c8f7e932b2b60 100644
--- a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -7653,4 +7653,20 @@ TEST_F(OpenMPIRBuilderTest, createGPUOffloadEntry) {
EXPECT_TRUE(Fn->hasFnAttribute(Attribute::MustProgress));
}
+TEST_F(OpenMPIRBuilderTest, splitBB) {
+ OpenMPIRBuilder OMPBuilder(*M);
+ OMPBuilder.Config.IsTargetDevice = false;
+ OMPBuilder.initialize();
+ F->setName("func");
+ IRBuilder<> Builder(BB);
+
+ Builder.SetCurrentDebugLocation(DL);
+ AllocaInst *alloc = Builder.CreateAlloca(Builder.getInt32Ty());
+ EXPECT_TRUE(DL == alloc->getStableDebugLoc());
+ BasicBlock *AllocaBB = Builder.GetInsertBlock();
+ splitBB(Builder, /*CreateBranch=*/true, "test");
+ if (AllocaBB->getTerminator())
+ EXPECT_TRUE(DL == AllocaBB->getTerminator()->getStableDebugLoc());
+}
+
} // namespace
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index 9c16388e3e348ff..48e4077ec2af64b 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -1392,7 +1392,8 @@ static llvm::Expected<llvm::BasicBlock *> allocateAndInitPrivateVars(
llvm::cast<llvm::BranchInst>(allocaIP.getBlock()->getTerminator());
splitBB(llvm::OpenMPIRBuilder::InsertPointTy(allocaIP.getBlock(),
allocaTerminator->getIterator()),
- true, "omp.region.after_alloca");
+ true, allocaTerminator->getStableDebugLoc(),
+ "omp.region.after_alloca");
llvm::IRBuilderBase::InsertPointGuard guard(builder);
// Update the allocaTerminator in case the alloca block was split above.
diff --git a/mlir/test/Target/LLVMIR/omptarget-debug-nowait.mlir b/mlir/test/Target/LLVMIR/omptarget-debug-nowait.mlir
new file mode 100644
index 000000000000000..eaa88d9dd60535e
--- /dev/null
+++ b/mlir/test/Target/LLVMIR/omptarget-debug-nowait.mlir
@@ -0,0 +1,40 @@
+// RUN: mlir-translate -mlir-to-llvmir %s
+
+module attributes {omp.is_target_device = false} {
+ llvm.func @main() {
+ %0 = llvm.mlir.constant(1 : i64) : i64
+ %1 = llvm.alloca %0 x f32 : (i64) -> !llvm.ptr
+ %3 = llvm.alloca %0 x i32 : (i64) -> !llvm.ptr
+ %6 = omp.map.info var_ptr(%1 : !llvm.ptr, f32) map_clauses(tofrom) capture(ByRef) -> !llvm.ptr
+ %7 = omp.map.info var_ptr(%3 : !llvm.ptr, i32) map_clauses(implicit, exit_release_or_enter_alloc) capture(ByCopy) -> !llvm.ptr
+ omp.target nowait map_entries(%6 -> %arg0, %7 -> %arg1 : !llvm.ptr, !llvm.ptr) {
+ %8 = llvm.mlir.constant(0 : i64) : i64
+ %9 = llvm.mlir.constant(100 : i32) : i32
+ llvm.br ^bb1(%9, %8 : i32, i64)
+ ^bb1(%13: i32, %14: i64): // 2 preds: ^bb0, ^bb2
+ %15 = llvm.icmp "sgt" %14, %8 : i64
+ llvm.cond_br %15, ^bb2, ^bb3
+ ^bb2: // pred: ^bb1
+ llvm.store %13, %arg1 : i32, !llvm.ptr
+ llvm.br ^bb1(%13, %14 : i32, i64)
+ ^bb3: // pred: ^bb1
+ llvm.store %13, %arg1 : i32, !llvm.ptr
+ omp.terminator
+ }
+ llvm.return
+ } loc(#loc2)
+}
+
+#file = #llvm.di_file<"test.f90" in "">
+#di_null_type = #llvm.di_null_type
+#cu = #llvm.di_compile_unit<id = distinct[0]<>,
+ sourceLanguage = DW_LANG_Fortran95, file = #file, isOptimized = false,
+ emissionKind = Full>
+#sp_ty = #llvm.di_subroutine_type<callingConvention = DW_CC_program,
+ types = #di_null_type>
+#sp = #llvm.di_subprogram<compileUnit = #cu, name = "main", file=#file,
+ subprogramFlags = "Definition", type = #sp_ty>
+
+#loc1 = loc("test.f90":6:7)
+#loc2 = loc(fused<#sp>[#loc1])
+
More information about the Mlir-commits
mailing list