[DebugInfo][RemoveDIs] Assert if we mix PHIs and debug-info #84054
Conversation
A potentially erronous code construction with the work we've done to remove
debug intrinsics, is inserting PHIs into blocks when the position hasn't
been "sourced correctly". Specifically, if you have:
%foo = PHI
#dbg_value
%bar = add i32...
And plan on inserting a new PHI, you have to use the iterator form of
getFirstNonPHI or getFirstInsertionPt (or begin()) to acquire an iterator
that tells the debug-info maintenence code "this is supposed to be at the
start of the block, put it in front of #dbg_value". We can detect
call-sites that aren't doing this at runtime, and should do with this
assertion. It might invalidate code that's doing something very unexpected,
like walking backwards to find a PHI, then going forwards, then inserting:
however that's just an inefficient way of calling getFirstNonPHI.
|
@llvm/pr-subscribers-llvm-ir Author: Jeremy Morse (jmorse) ChangesA potentially erroneous code construction with the work we've done to remove debug intrinsics, is inserting PHIs into blocks when the position hasn't been "sourced correctly". Specifically, if you have: And plan on inserting a new PHI, you have to use the iterator form of ~ While we can detect this at runtime, it's sort of a sticking plaster. It seems like we might have to end up converting Full diff: https://github.com/llvm/llvm-project/pull/84054.diff 1 Files Affected:
diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index ce221758ef798b..e863ef3eb8d6d7 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -149,6 +149,18 @@ void Instruction::insertBefore(BasicBlock &BB,
if (!InsertAtHead) {
DPMarker *SrcMarker = BB.getMarker(InsertPos);
if (SrcMarker && !SrcMarker->empty()) {
+ // If this assertion fires, the calling code is about to insert a PHI
+ // after debug-records, which would form a sequence like:
+ // %0 = PHI
+ // #dbg_value
+ // %1 = PHI
+ // Which is de-normalised and undesired -- hence the assertion. To avoid
+ // this, you must insert at that position using an iterator, and it must
+ // be aquired by calling getFirstNonPHIIt / begin or similar methods on
+ // the block. This will signal to this behind-the-scenes debug-info
+ // maintenence code that you intend the PHI to be ahead of everything,
+ // including any debug-info.
+ assert(!isa<PHINode>(this) && "Inserting PHI after debug-records!");
adoptDbgValues(&BB, InsertPos, false);
}
}
|
|
Testing: this compiles stage2reldeb clang successfully. I've also today merged all our modifications of PHI-creation sites to always use iterators, so it Shouldn't (TM) fire at all. |
|
@OCHyams could you land this for me, as I'm not able to monitor the buildbots tomorrow? |
A potentially erroneous code construction with the work we've done to remove debug intrinsics, is inserting PHIs into blocks when the position hasn't been "sourced correctly". Specifically, if you have:
And plan on inserting a new PHI, you have to use the iterator form of
getFirstNonPHIor getFirstInsertionPt (or begin()) to acquire an iterator that tells the debug-info maintenance code "this is supposed to be at the start of the block, put it in front of #dbg_value". We can detect call-sites that aren't doing this at runtime, and should do with this assertion. It might invalidate code that's doing something very unexpected, like walking backwards to find a PHI, then going forwards, then inserting: however that's just an inefficient way of callinggetFirstNonPHI.~
While we can detect this at runtime, it's sort of a sticking plaster. It seems like we might have to end up converting
getFirstNonPHIto return an iterator, which is going to be annoying (128 call-sites in llvm/lib/Transforms). However this is all the natural consequence of putting debug-info correctness into the type system which is why we're doing this anyway.