[RemoveDIs] Enable conversion from dbg.declare to DPValue #74090
Conversation
|
@llvm/pr-subscribers-llvm-ir Author: Orlando Cazalet-Hyams (OCHyams) ChangesThe testing situation here is a little bit difficult. Most of the dbg.declare support patches depend on this patch to be able to test them. However, landing this patch first would result in ~10-15 test failures in tests that already Options:
Any strong opinions on the options above? I have listed them in order of personal preference (1 being most preferred), but am happy to go with any as they all have pros & cons. Full diff: https://github.com/llvm/llvm-project/pull/74090.diff 1 Files Affected:
diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
index 0a8fddbe102538d..eefa46a581f6185 100644
--- a/llvm/lib/IR/BasicBlock.cpp
+++ b/llvm/lib/IR/BasicBlock.cpp
@@ -73,7 +73,10 @@ void BasicBlock::convertToNewDbgValues() {
SmallVector<DPValue *, 4> DPVals;
for (Instruction &I : make_early_inc_range(InstList)) {
assert(!I.DbgMarker && "DbgMarker already set on old-format instrs?");
- if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(&I)) {
+ if (DbgVariableIntrinsic *DVI = dyn_cast<DbgVariableIntrinsic>(&I)) {
+ if (isa<DbgAssignIntrinsic>(DVI))
+ continue;
+
// Convert this dbg.value to a DPValue.
DPValue *Value = new DPValue(DVI);
DPVals.push_back(Value);
|
There was a problem hiding this comment.
Overall idea LGTM. In terms of landing things, we've made a mess by having this partial overlapping coverage sadly, so there'll need to be some kind of gap for some period.
An additional constraint however is that there /might/ now be people testing this code, seeing how we've announced testing, and if we land the dbg-declare-conversion code up front, there might be some spurious differences / crashes / whatever in the meantime. I'd prefer option 2 to avoid punishing anyone who's kind enough to test for us.
(Or add another command line flag that lasts two days and turns on dbg-declare conversion? All bad options sorry).
This is a boring mechanical update to support DPValues that look like dbg.declares in SelectionDAG. The tests will become "live" once #74090 lands (see for more info).
The tests will become "live" once #74090 lands (see for more info).
) The intrinsic variants of these functions don't do anything to dbg.declares so the non-instruction variants should ignore them too. Tested in llvm/test/DebugInfo/duplicate_dbgvalue.ll, which has `--try-experimental-debuginfo-iterators` added in #73504. The tests will become "live" once #74090 lands (see for more info).
That patch was missing a recent change to the non-DPValue StoreInst overload. Add it to the DPValue version. This is tested by `llvm/tests/Transforms/Mem2Reg/dbg_declare_to_value_conversions.ll`, which already has `--try-experimental-debuginfo-iterators`. It doesn't fail currently because until #74090 lands the declares are not converted to DPValues. The tests will become "live" once #74090 lands (see for more info).
The tests will become "live" once #74090 lands (see for more info).
Handle dbg.declares in SROA using DPValues. In order to reduce duplication, the migrate-debug-info loop has been changed to a generic lambda with some helper function overloads, which is called for dbg.declares, dbg.assigns, and DPValues alike. The tests will become "live" once #74090 lands (see for more info).
|
I've gone with option 2 as you've suggested. That makes it easy to temporarily disable this "capstone" patch as a fix-call too if problems are found. |
As part of the RemoveDIs project, transitioning to non-instruction debug info, all debug intrinsic handling code needs to be duplicated to handle DPValues. --try-experimental-debuginfo-iterators enables the new debug mode in tests if the CMake option has been enabled. `getInsertPtAfterFramePtr` now returns an iterator so we don't lose debug-info-communicating bits. --- Depends on #73500, #74090, #74091.
…gfx:ac9bf99e79f1 Local branch amd-gfx ac9bf99 Merged main:ef112833e11e94ea049f98bec4a29b4fe96a25dd into amd-gfx:d621799ca22b Remote branch main bdbc2db [RemoveDIs] Enable conversion from dbg.declare to DPValue (llvm#74090) Change-Id: Ic1c577a48c31052c9e44ce3733adc8b71a132127
The testing situation here is a little bit difficult. Most of the dbg.declare support patches depend on this patch to be able to test them. However, landing this patch first would result in ~10-15 test failures in tests that already
--try-experimental-debuginfo-iteratorsthat contain both dbg.values (already fully supported and being tested) and dbg.declares (support in flight: shouldn't really be tested yet - those updates would come as support is added to each pass etc, as we did with dbg.values, if that were possible).Options:
--try-experimental-debuginfo-iteratorsfrom those failing tests in this patch, re-add as those passes get dbg.declare support/fixes.Any strong opinions on the options above? I have listed them in order of personal preference (1 being most preferred), but am happy to go with any as they all have pros & cons.