[llvm] [OMPIRBuilder] Avoid crash in BasicBlock::splice. (PR #154987)

[Abid Qadeer via llvm-commits]

https://github.com/abidh updated https://github.com/llvm/llvm-project/pull/154987

>From f8f9835fd09d5e5bff4260474685216c893ab5ee Mon Sep 17 00:00:00 2001
From: Abid Qadeer <haqadeer at amd.com>
Date: Fri, 22 Aug 2025 15:45:02 +0100
Subject: [PATCH 1/4] [OMPIRBuilder] Avoid crash in BasicBlock::splice.

Calling splice when both Old and New are empty is a nop currently but
it can cause a crash once debug records are used instead of debug
intrinsics. This PR makes the call conditional on at least one of
Old or New being non-empty.

Consider the following mlir:
omp.target map_entries() {
  llvm.intr.dbg.declare ...
  llvm.intr.dbg.declare ...
  omp.teams ...
  ...
}

Current code would translate llvm.intr to llvm intrinsics and we will
have 2 of them in the Old BB by the time omp.teams implementation starts.
This implementation creates many BasicBlocks by calling splitBB.

In the new scheme (using debug records), there will be no instruction
in the Old BB after llvm.intr get translated but just 2 trailing debug
records. So effectively both Old and New are empty. When control reaches
BasicBlock::splice, it calls spliceDebugInfoEmptyBlock. This funtion
expects that in this case (Src is empty but has trailing debug records),
the ToIt is valid and it can call adoptDbgRecords on it. This
assumption is not true in this case as New is empty and ToIt is pointing
to end().
---
 llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index c0e956840f989..f6afa93f21bdc 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -307,7 +307,8 @@ void llvm::spliceBB(IRBuilderBase::InsertPoint IP, BasicBlock *New,
 
   // Move instructions to new block.
   BasicBlock *Old = IP.getBlock();
-  New->splice(New->begin(), Old, IP.getPoint(), Old->end());
+  if (!Old->empty() || !New->empty())
+    New->splice(New->begin(), Old, IP.getPoint(), Old->end());
 
   if (CreateBranch) {
     auto *NewBr = BranchInst::Create(New, Old);

>From 3654d526ef83fc6b0c30dfe47a955f85f5e8acd8 Mon Sep 17 00:00:00 2001
From: Abid Qadeer <haqadeer at amd.com>
Date: Tue, 26 Aug 2025 15:46:05 +0100
Subject: [PATCH 2/4] Add a unit test.

---
 .../Frontend/OpenMPIRBuilderTest.cpp          | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
index b7a060bb3563d..c13570dc803b3 100644
--- a/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
+++ b/llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
@@ -7852,4 +7852,28 @@ TEST_F(OpenMPIRBuilderTest, splitBB) {
     EXPECT_TRUE(DL == AllocaBB->getTerminator()->getStableDebugLoc());
 }
 
+TEST_F(OpenMPIRBuilderTest, spliceBBWithEmptyBB) {
+  OpenMPIRBuilder OMPBuilder(*M);
+  OMPBuilder.Config.IsTargetDevice = false;
+  OMPBuilder.initialize();
+  F->setName("func");
+  IRBuilder<> Builder(BB);
+
+  // Test calling spliceBB with an empty Block (but having trailing debug
+  // records).
+  DIBuilder DIB(*M);
+  DISubprogram *SP = F->getSubprogram();
+  DIType *VoidPtrTy =
+      DIB.createQualifiedType(dwarf::DW_TAG_pointer_type, nullptr);
+  DILocalVariable *Var = DIB.createParameterVariable(
+      SP, "test", /*ArgNo*/ 1, SP->getFile(), /*LineNo=*/0, VoidPtrTy);
+  DIB.insertDeclare(F->getArg(0), Var, DIB.createExpression(), DL,
+                    Builder.GetInsertPoint());
+  BasicBlock *New = BasicBlock::Create(Ctx, "", F);
+  spliceBB(Builder.saveIP(), New, true, DL);
+  Instruction *Terminator = BB->getTerminator();
+  EXPECT_NE(Terminator, nullptr);
+  EXPECT_FALSE(Terminator->getDbgRecordRange().empty());
+}
+
 } // namespace

>From 6e02d884f10e649fcf149186f5517b0e1abc42c7 Mon Sep 17 00:00:00 2001
From: Abid Qadeer <haqadeer at amd.com>
Date: Fri, 29 Aug 2025 15:50:24 +0100
Subject: [PATCH 3/4] Avoid splice call only when Old is empty.

This removes the inconsistent behavior in the PR. Also added comments to describe the rationale.
---
 llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index f6afa93f21bdc..155ca39959c50 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -307,7 +307,14 @@ void llvm::spliceBB(IRBuilderBase::InsertPoint IP, BasicBlock *New,
 
   // Move instructions to new block.
   BasicBlock *Old = IP.getBlock();
-  if (!Old->empty() || !New->empty())
+  // If the Old block is empty then there are no instructions to move. But in
+  // the new debug scheme, it could have trailing debug records which will be
+  // moved to New in spliceDebugInfoEmptyBlock. We dont want that for 2 reasons:
+  // 1. If New is also empty, it could cause a crash.
+  // 2. Even if New is not empty, we want to keep those debug records in the
+  // Old as that was the behavior with the old scheme (debug intrinsics).
+  // So we call BasicBlock::splice only when Old is not empty.
+  if (!Old->empty())
     New->splice(New->begin(), Old, IP.getPoint(), Old->end());
 
   if (CreateBranch) {

>From 70bacd06d59c9805031f1c0d4db11230e239bd90 Mon Sep 17 00:00:00 2001
From: Abid Qadeer <haqadeer at amd.com>
Date: Fri, 29 Aug 2025 16:20:15 +0100
Subject: [PATCH 4/4] Update comments.

---
 llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index 155ca39959c50..e740c2819fec9 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -307,13 +307,17 @@ void llvm::spliceBB(IRBuilderBase::InsertPoint IP, BasicBlock *New,
 
   // Move instructions to new block.
   BasicBlock *Old = IP.getBlock();
-  // If the Old block is empty then there are no instructions to move. But in
+  // If the `Old` block is empty then there are no instructions to move. But in
   // the new debug scheme, it could have trailing debug records which will be
-  // moved to New in spliceDebugInfoEmptyBlock. We dont want that for 2 reasons:
-  // 1. If New is also empty, it could cause a crash.
-  // 2. Even if New is not empty, we want to keep those debug records in the
-  // Old as that was the behavior with the old scheme (debug intrinsics).
-  // So we call BasicBlock::splice only when Old is not empty.
+  // moved to `New` in `spliceDebugInfoEmptyBlock`. We dont want that for 2
+  // reasons:
+  // 1. If `New` is also empty, `BasicBlock::splice` crashes.
+  // 2. Even if `New` is not empty, the rationale to move those records to `New`
+  // (in `spliceDebugInfoEmptyBlock`) does not apply here. That function
+  // assumes that `Old` is optimized out and is going away. This is not the case
+  // here. The `Old` block is still being used e.g. a branch instruction is
+  // added to it later in this function.
+  // So we call `BasicBlock::splice` only when `Old` is not empty.
   if (!Old->empty())
     New->splice(New->begin(), Old, IP.getPoint(), Old->end());