Page MenuHomePhabricator

[Debuginfo] (3/N) Support for DW_OP_implicit_pointer for named and unnamed variables (second strategy).
Needs ReviewPublic

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

Details

Reviewers
dblaikie
Summary
This patch stems from D84112.
This patch includes changes for CodeGen phase.

Diff Detail

Unit TestsFailed

TimeTest
130 mswindows > LLVM.DebugInfo/ARM::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; c:\ws\w3\llvm-project\premerge-checks\build\bin\opt.exe -codegenprepare -S C:\ws\w3\llvm-project\premerge-checks\llvm\test\DebugInfo\ARM\salvage-debug-info.ll -o - | c:\ws\w3\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w3\llvm-project\premerge-checks\llvm\test\DebugInfo\ARM\salvage-debug-info.ll
130 mswindows > LLVM.Transforms/CodeGenPrepare::Unknown Unit Message ("")
Script: -- : 'RUN: at line 2'; c:\ws\w3\llvm-project\premerge-checks\build\bin\opt.exe < C:\ws\w3\llvm-project\premerge-checks\llvm\test\Transforms\CodeGenPrepare\sink-shift-and-trunc.ll -codegenprepare -mtriple=arm64-apple-ios -S | c:\ws\w3\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w3\llvm-project\premerge-checks\llvm\test\Transforms\CodeGenPrepare\sink-shift-and-trunc.ll

Event Timeline

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

I sense this is the bulk of the work; and it looks good. I've posted on llvm-dev asking whether the creation of artificial variables could instead be deferred til DWARF emission. That'll avoid anything having to deal with a new form of DBG_VALUE, and us having to think about their lifetimes. Plus, there's greater scope for de-duplication, as I understand it this patch will generate a new artificial variable for each dbg.value, for each level of indirection.

On that topic -- do we need to give the artificial variable a name? I'm no DWARF expert, but can we just not attach a DW_AT_name to the DIE?

llvm/include/llvm/CodeGen/MachineInstrBuilder.h
240–242

I think the preferred form is !MI->isDebugValue() || static_cast...., i.e., the assertion always passes if the MI isn't a DBG_VALUE, or if it is, the getDebugImplicitVariable() part is tested. This avoids the reader having to consider an un-necessary ternary operator.

244–246

(Same as above)

llvm/include/llvm/IR/DIBuilder.h
619

This'll want a docu-comment as to createImplicitTempVariable's purpose.

llvm/include/llvm/IR/DebugInfoMetadata.h
2540

nit: Full stops at ends of comments, here and elsewhere.

2546

"indirection"

llvm/include/llvm/IR/IntrinsicInst.h
101

Doc comment needed too please.

llvm/lib/CodeGen/AsmPrinter/DebugLocEntry.h
67–69

Shouldn't this be in the union?

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
81–83
84

Would it be possible to use a subtype of DINode instead, such as DIVariable? That'll increase type safety and make it clearer what's going on.

133

I'd suggest the return type can be auto here.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2518

I reckon it's better to assert(Die);, there should be no scenarios where the implicit pointer DIE isn't emitted, right? Otherwise, if this if condition is false, you'll write the unmodified ValOffset into the output dwarf, which could be misleading.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
1323–1324

NB: this function definitely needs some comments in it as to what's going on.

1331

Better to test for Level==0 and have the normal-dbg-value case return early; then the rest of this function doesn't need an extra level of indentation / scope.

1390

Correct me if I'm wrong, but isn't generation of DW_OP_LLVM_implicit_pointer only done during instruction selection? If so, can we not reject implicit_pointer expressions at this stage as being illegal IR?

llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
117–119

It looks like this contradicts the immediately preceeding comment, could you update the comment please?

llvm/test/DebugInfo/X86/dwarfdump-implicit_pointer_instcomb.ll
4–12

Don't both ptr and ptrVar become implicit pointers? Does this lead to two implicit_ptr_tmp variables being created?

It'd also be good to capture/compare the operand to DW_OP_implicit_pointer to the DIE address that it refers to, to confirm that it points to the correct place.

7

CHECK-NEXT: DW_AT_name "ptrVar"?

llvm/test/DebugInfo/X86/dwarfdump-implicit_pointer_mem2reg.ll
5

Again, in this test it would be good to ensure that DW_OP_implicit_pointer points at the correct DIE. If the intention here is to only test for multiple levels of implicit pointer, it's probably sufficient to only check one variable.

llvm/test/DebugInfo/X86/dwarfdump-implicit_pointer_sroa.ll
3

It'd be good to test for non-zero offsets into an implicit variable -- i.e., in the original C, have ptr3 something like:

ptr3 = &arr1[1];
llvm/test/DebugInfo/X86/dwarfdump-implicit_pointer_sroa_inline.ll
7–11

So, there'll be a DW_OP_implicit_pointer for each value a pointer-variable can point at, rather than one DW_OP_implicit_pointer and two entries in the variable/location-list it refers to?

(This might not be the most efficient way of doing it; maybe it's sufficient for now though).