[DebugInfo] Set all dbg.value intrinsics to be tail-calls #73661
Conversation
This change has no meaningful effect on the compiler, although it has a functional effect of dbg.value intrinsics being printed differently. The tail-call flag is meaningless for debug-intrinsics and doesn't serve a purpose, it's just extra baggage that dbg.values are built on top of. Some facilities create debug-intrinsics with the flag, others don't. However, the RemoveDIs project to represent debug-info without intrinsics doesn't have a corresponding flag, which can cause spurious test differences. Specifically: we can convert a dbg.value to a DPValue, run an optimisation pass, then convert the DPValue back to dbg.value form. Right now, we always set the "tail" flag when converting it back. This causes the auto-update-tests script to fail sometimes because in one mode (dbg.value) intrinsics might not have a tail flag, but in the other they do have a tail flag. Consistently picking one or the other in the conversion routine doesn't help, because the rest of LLVM is inconsistent about it anyway. Thus: whenever we make a dbg.value intrinsic, create it as a tail call, so that we get consistent output behaviours no matter which debug-info mode we're in, DPValue or dbg.value. No tests fail as a result of this patch because the extra 'tail' generated in numerous tests is automatically ignored by FileCheck as being leading-rubbish before the CHECK match.
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-debuginfo Author: Jeremy Morse (jmorse) ChangesThis change has no meaningful effect on the compiler, although it has a functional effect of dbg.value intrinsics being printed differently. The tail-call flag is meaningless for debug-intrinsics and doesn't serve a purpose, it's just extra baggage that dbg.values are built on top of. Some facilities create debug-intrinsics with the flag, others don't. However, the RemoveDIs project to represent debug-info without intrinsics doesn't have a corresponding flag, which can cause spurious test differences. Specifically: we can convert a dbg.value to a DPValue, run an optimisation pass, then convert the DPValue back to dbg.value form. Right now, we always set the "tail" flag when converting it back. This causes the auto-update-tests script to fail sometimes because in one mode (dbg.value) intrinsics might not have a tail flag, but in the other they do have a tail flag. Consistently picking one or the other in the conversion routine doesn't help, because the rest of LLVM is inconsistent about it anyway. Thus: whenever we make a dbg.value intrinsic, create it as a tail call, so that we get consistent output behaviours no matter which debug-info mode we're in, DPValue or dbg.value. No tests fail as a result of this patch because the extra 'tail' generated in numerous tests is automatically ignored by FileCheck as being leading-rubbish before the CHECK match. Full diff: https://github.com/llvm/llvm-project/pull/73661.diff 1 Files Affected:
diff --git a/llvm/lib/IR/DIBuilder.cpp b/llvm/lib/IR/DIBuilder.cpp
index 0946cf8048f241a..62efaba025344b5 100644
--- a/llvm/lib/IR/DIBuilder.cpp
+++ b/llvm/lib/IR/DIBuilder.cpp
@@ -988,9 +988,11 @@ Instruction *DIBuilder::insertDbgValueIntrinsic(Value *V,
DIExpression *Expr,
const DILocation *DL,
Instruction *InsertBefore) {
- return insertDbgValueIntrinsic(
+ Instruction *DVI = insertDbgValueIntrinsic(
V, VarInfo, Expr, DL, InsertBefore ? InsertBefore->getParent() : nullptr,
InsertBefore);
+ cast<CallInst>(DVI)->setTailCall();
+ return DVI;
}
Instruction *DIBuilder::insertDbgValueIntrinsic(Value *V,
|
| V, VarInfo, Expr, DL, InsertBefore ? InsertBefore->getParent() : nullptr, | ||
| InsertBefore); | ||
| cast<CallInst>(DVI)->setTailCall(); |
There was a problem hiding this comment.
Would it be better to put this change in DIBuilder::insertDbgIntrinsic, which seems to be the common leaf between all the insertDbgXIntrinsic functions (that's where the IRBuilder::CreateCall call is)?
There was a problem hiding this comment.
Probably in the long term (if dbg.values had a long term), however I think that's common between all DbgVariableIntrinsics, thus including dbg.declare too, which I don't want to get into at this stage.
There was a problem hiding this comment.
It seems harmless to cover dbg.declares too to me, given
This change has no meaningful effect on the compiler
But I won't insist
This change has no meaningful effect on the compiler, although it has a functional effect of dbg.value intrinsics being printed differently. The tail-call flag is meaningless for debug-intrinsics and doesn't serve a purpose, it's just extra baggage that dbg.values are built on top of. Some facilities create debug-intrinsics with the flag, others don't. However, the RemoveDIs project to represent debug-info without intrinsics doesn't have a corresponding flag, which can cause spurious test differences.
Specifically: we can convert a dbg.value to a DPValue, run an optimisation pass, then convert the DPValue back to dbg.value form. Right now, we always set the "tail" flag when converting it back. This causes the auto-update-tests script to fail sometimes because in one mode (dbg.value) intrinsics might not have a tail flag, but in the other they do have a tail flag. Consistently picking one or the other in the conversion routine doesn't help, because the rest of LLVM is inconsistent about it anyway.
Thus: whenever we make a dbg.value intrinsic, create it as a tail call, so that we get consistent output behaviours no matter which debug-info mode we're in, DPValue or dbg.value. No tests fail as a result of this patch because the extra 'tail' generated in numerous tests is automatically ignored by FileCheck as being leading-rubbish before the CHECK match.