This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Remove `dbg.addr` from CodeGen
ClosedPublic

Authored by jryans on Feb 25 2023, 4:13 PM.

Details

Summary

As part of this work, removing SDDbgValue::clearIsEmitted originally added for
dbg.addr in 045c67769d7fe577fc38cccb6fb40fd814437447 was attempted, but it
appears some tests for DBG_INSTR_REF now depend on that behaviour as well, so
it was kept and comments were updated instead.

Part of dbg.addr removal
Discussed in https://discourse.llvm.org/t/what-is-the-status-of-dbg-addr/62898

Depends on D144799

Diff Detail

Event Timeline

jryans created this revision.Feb 25 2023, 4:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2023, 4:13 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
jryans requested review of this revision.Feb 25 2023, 4:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2023, 4:13 PM
jryans retitled this revision from [DebugInfo] Remove `dbg.addr` from CodeGen r=Orlando,StephenTozer,probinson,rnk to [DebugInfo] Remove `dbg.addr` from CodeGen.Feb 25 2023, 4:18 PM
jryans added a project: debug-info.
StephenTozer accepted this revision.Feb 27 2023, 2:28 AM
StephenTozer added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
6090

👍

llvm/test/DebugInfo/X86/merge-equivalent-ranges.ll
31

For the purposes of this test, we can also remove this line and the test for "Var1" above - since this test is testing that this range merges with the one that is being deleted, the test isn't meaningful anymore; ideally we'd have a good way to test this otherwise, but don't consider it a blocker on this patch.

This revision is now accepted and ready to land.Feb 27 2023, 2:28 AM
jryans marked an inline comment as done.Feb 27 2023, 7:57 AM
jryans added inline comments.
llvm/test/DebugInfo/X86/merge-equivalent-ranges.ll
31

Ah hmm, what if instead of removing the dbg.addr below, I were to replace it with dbg.declare...? Would that preserve the full meaning of this test?

StephenTozer added inline comments.Feb 27 2023, 8:46 AM
llvm/test/DebugInfo/X86/merge-equivalent-ranges.ll
31

If a dbg.declare exists for a variable, there should be no other debug intrinsics for that variable; I'm not sure if that would be validated, but it *shouldn't* happen naturally, so best not to test against it imo.

jryans added inline comments.Feb 27 2023, 8:52 AM
llvm/test/DebugInfo/X86/merge-equivalent-ranges.ll
31

Ah hmm, perhaps then we could repeat the dbg.value + deref from line 31 at the removed line 40 (effectively that's what the auto-upgraded dbg.addr would turn into anyway)...?

StephenTozer added inline comments.Feb 27 2023, 9:01 AM
llvm/test/DebugInfo/X86/merge-equivalent-ranges.ll
31

Also sadly not - the difference with dbg.addr is that we have DBG_VALUE %r, $noreg, !"var1", !DIExpr(DW_OP_deref) and DBG_VALUE %r, 0, !"var1", !DIExpr(); identical meanings, but different representations. That's what's being tested for here - that at some point the compiler recognizes the debug values mean the same thing and merges them. I wouldn't worry about this test blocking the patch - the right move might be to convert this to a MIR test with the instructions being explicit, but that can be done separately.

jryans updated this revision to Diff 501073.Feb 28 2023, 3:33 AM
jryans edited the summary of this revision. (Show Details)

Removed surrounding test checks following review comments

jryans marked 3 inline comments as done.Feb 28 2023, 3:33 AM
This revision was landed with ongoing or failed builds.Mar 2 2023, 1:32 AM
This revision was automatically updated to reflect the committed changes.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp