This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Remove `dbg.addr` from CodeGen

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



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

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.



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.

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

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

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

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.