[OMPIRBuilder] Avoid crash in BasicBlock::splice. #154987
Conversation
|
@llvm/pr-subscribers-flang-openmp Author: Abid Qadeer (abidh) ChangesCalling Consider the following mlir: Current code would translate llvm.intr Ops to llvm intrinsics. Old is the BasicBlock where they were get inserted and it will have 2 llvm debug intrinsics by the time the implementation of In the new scheme (using debug records), there will be no instruction in the Full diff: https://github.com/llvm/llvm-project/pull/154987.diff 1 Files Affected:
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index 27a4fcfb303c4..2ff6d3a8ddd21 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -308,7 +308,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);
|
|
I don't follow what is happening. Why doesn't Can you add a unittest for illustration? I don't mean a translation from MLIR, but some IRBuilder-generated LLVM-IR on which |
The I added a unit test that show the scenario where the |
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().
|
Thank you for the unittest, it helps me understand the issue. It seems that I think that |
I agree that The call to In the new scheme, there are no intrinsics so the the |
|
The changed behavior comes from the addition of "trailing DbgRecords" with the new scheme. If they are meant to stay at the beginning of the BB they should be called "leading DbgRecords". If the were meant to stay in the source BB then Footnotes
|
|
I agree with your point that moving dbg records from I am a bit hesitant about adding |
This removes the inconsistent behavior in the PR. Also added comments to describe the rationale.
| // 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. |
There was a problem hiding this comment.
| // 1. If New is also empty, it could cause a crash. | |
| // 1. If New is also empty, `BasicBlock::splice` crashes |
[nit] to be specific. If it doesn't crash anymore, we know it has been fixed.
| // 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). |
There was a problem hiding this comment.
| // 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). | |
| // 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). |
[nit] Just being the legacy scheme doesn't mean it is more correct. Consider something that mentions the rationale from BasicBlock::spliceDebugInfoEmptyBlock:
// If the source block is completely empty, including no terminator, then
// transfer any trailing DbgRecords that are still hanging around. This can
// occur when a block is optimised away and the terminator has been moved
// somewhere else.
which does not apply here since we still want to use Old BB (like adding a BranchInst with CreateBranch). And/or that those trailing DbgRecords should only exist if Old is empty since there is no other place to attach them.
Calling
BasicBlock::spliceinspliceBBwhen bothOldandNeware empty is anopcurrently 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 ofOldorNewbeing non-empty.Consider the following mlir:
Current code would translate llvm.intr Ops to llvm intrinsics. Old is the BasicBlock where they were get inserted and it will have 2 llvm debug intrinsics by the time the implementation of
omp.teamsstarts. This implementation creates many BasicBlocks by callingsplitBB. TheNewis the just created BasicBlock which is empty.In the new scheme (using debug records), there will be no instruction in the
OldBB after llvm.intr Ops get translated but just 2 trailing debug records. So bothOldandNeware empty. When control reachesBasicBlock::splice, it callsspliceDebugInfoEmptyBlock. This function expects that in this case (Srcis empty but has trailing debug records), theToItis valid and it can calladoptDbgRecordson it. This assumption is not true in this case asNewis empty andToItis pointing to end(). The fix is to only callBasicBlock::splicewhen at least ofOldorNewis not empty.