This is an archive of the discontinued LLVM Phabricator instance.

[Debuginfo][mem2reg] (6/7) Support for DW_OP_implicit_pointer for named and unnamed variables (second strategy).
Needs ReviewPublic

Authored by alok on Jul 18 2020, 5:11 PM.

Details

Summary
This patch stems from D84112.
This patch includes changes for mem2reg optimization.

Diff Detail

Event Timeline

alok created this revision.Jul 18 2020, 5:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2020, 5:11 PM
jmorse added a subscriber: jmorse.Jul 30 2020, 6:04 AM

Suggested on llvm-dev: I reckon you also need to add logic to the ConvertDebugDeclareToDebugValue helpers too, or otherwise factor them in. This patch only covers the two fast-cases that mem2reg considers, single store allocas and single block allocas, I believe (98%) everything else goes through ConvertDebugDeclareToDebugValue.

The tests should also be IR tests that run a single pass (mem2reg) and ensure they do the right thing for a fixed IR input; anything else is vulnerable to changes elsewhere in the compiler.

llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp
421

All this new code added will need comments added explaining the intention behind it; it's hard to review otherwise.

434–437

This is a side-note rather than a review comment:

As I understand it, this is ensuring that if someone writes:

dbg.value(%ptr, !123, !DIExpression(DW_OP_deref))

Then we just make the variable refer to the promoted value, rather than making it an implicit pointer. IMO, this isn't something we truly need to support, as a dbg.value like that is is already faulty, see PR40628 [0], they're vulnerable to store re-ordering.

(The overall solution to DW_OP_deref in dbg.values is probably to have the IR verifyer deny them).

https://bugs.llvm.org/show_bug.cgi?id=40628

547–552

StoresByIndex isn't modified after creation; why does it need to be cleared and re-created here?

577

(The disabled code here isn't for review, right)?

601–619

It seems a fair amount of this is shared with the single-value promoter, it can probably be moved to a helper function.

620

It's unclear what the code below is doing without a comment.

llvm/test/DebugInfo/X86/dwarfdump-implicit_pointer_instcomb.c
1–2

As with patch six, we can't call clang from llvm tests, this needs to be tested by using "opt" on unoptimized IR. Because this is testing an IR pass, it should test that the correct LLVM-IR output is produced, not an object file.

I agree there should be some kind of end-to-end test; as said in patch six, this is best done in debug-info / Dexter tests, where we can ensure that the debugging experience is as expected, without being too specific about how it's done (which introduces a maintenence burden).

alok updated this revision to Diff 304754.Nov 12 2020, 1:57 AM
alok retitled this revision from [Debuginfo] (7/N) Support for DW_OP_implicit_pointer for named and unnamed variables (second strategy). to [Debuginfo][mem2reg] (6/7) Support for DW_OP_implicit_pointer for named and unnamed variables (second strategy)..

Updated to re-base.