[PATCH] D104520: [InstrRef][AArch64][1/4] Accept constant physreg variable locations

[Jeremy Morse via Phabricator via llvm-commits]

jmorse created this revision.
jmorse added a reviewer: debug-info.
Herald added subscribers: danielkiss, hiraditya, kristof.beyls.
jmorse requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Meta: This is a series of patches that improve variable location coverage on aarch64 when using the instruction referencing model, by tracking stack spill slots and recovering instruction numbers when optimisations happen. My aim here is to make the new model more useful / accessible for the other frequently-used targets in LLVM so that it's easier to adopt.

In my experiments so far, a stage2reldebinfo build of clang has 5% more PC varible location coverage with instruction referencing (57% -> 62%), some of which will be additional entry values. Either way it's a good improvement. Of the variable locations that are dropped by instruction referencing but not by DBG_VALUEs, they're largely due to the instruction scheduler passes shuffling DBG_PHI instructions to refer to the wrong value definition. This should be easily solvable, but I haven't got around to it yet.

This patch:

Late in SelectionDAG we join up instruction numbers with their defining instructions, if it couldn't be done during the main part of SelectionDAG. One exception is function arguments, where we have to point a DBG_PHI instruction at the incoming live register, as they don't have a defining instruction. This patch adds another exception, for constant physregs, like aarch64 has.

Technically, constant physreg values do have a defining instruction, and we could represent them like this:

  %0 = COPY $wzr, debug-instr-number 1 
  DBG_INSTR_REF 1, 0 

However the copy is susceptible to being optimised out or folded through trivial def rematerialisation. Instead, because we have determined the final value of the variable (i.e., the constant physreg) we can use a DBG_PHI:

  DBG_PHI $wzr, 1 
  DBG_INSTR_REF 1, 0 

After which the location cannot ever be optimised away.

It may seem wasteful to use two instructions where we could use a single DBG_VALUE, however the whole point of instruction referencing is to decouple the identification of values from the specification of where variable location ranges start.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104520

Files:
  llvm/lib/CodeGen/MachineFunction.cpp
  llvm/test/DebugInfo/AArch64/instr-ref-const-physreg.ll


Index: llvm/test/DebugInfo/AArch64/instr-ref-const-physreg.ll
===================================================================
--- /dev/null
+++ llvm/test/DebugInfo/AArch64/instr-ref-const-physreg.ll
@@ -0,0 +1,38 @@
+; RUN: llc %s -stop-before=finalize-isel -march=aarch64 -o - \
+; RUN:     -experimental-debug-variable-locations | FileCheck %s
+
+; Test that when an SSA Value becomes a constant-physreg copy, under the
+; instruction referencing model, the COPY is labelled. In the general case
+; labelling a copy is undesirable -- this test really checks that we don't
+; crash, and we don't just drop the information.
+
+; CHECK: DBG_PHI $xzr, 1
+; CHECK: DBG_INSTR_REF 1, 0
+
+define i64 @test() !dbg !7 {
+  %foo = add i64 0, 0
+  call void @llvm.dbg.value(metadata i64 %foo, metadata !12, metadata !DIExpression()), !dbg !13
+  ret i64 %foo, !dbg !13
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "test.c", directory: "/tmp/out.c")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!""}
+!7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 3, type: !8, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10, !11, !11}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !DIBasicType(name: "long int", size: 64, encoding: DW_ATE_signed)
+!12 = !DILocalVariable(name: "bar", arg: 1, scope: !7, file: !1, line: 3, type: !11)
+!13 = !DILocation(line: 0, scope: !7)
+!14 = !DILocalVariable(name: "baz", arg: 2, scope: !7, file: !1, line: 3, type: !11)
Index: llvm/lib/CodeGen/MachineFunction.cpp
===================================================================
--- llvm/lib/CodeGen/MachineFunction.cpp
+++ llvm/lib/CodeGen/MachineFunction.cpp
@@ -1124,15 +1124,24 @@
     }
   }
 
+  MachineBasicBlock &InsertBB = *CurInst->getParent();
+
   // We reached the start of the block before finding a defining instruction.
-  // It must be an argument: assert that this is the entry block, and produce
-  // a DBG_PHI.
-  assert(!State.first.isVirtual());
-  MachineBasicBlock &TargetBB = *CurInst->getParent();
-  assert(&*TargetBB.getParent()->begin() == &TargetBB);
+  // It could be from a constant register, otherwise it must be an argument.
+  if (TRI.isConstantPhysReg(State.first)) {
+    // We can produce a DBG_PHI that identifies the constant physreg. Doesn't
+    // matter where we put it, as it's constant valued.
+    assert(CurInst->isCopy());
+  } else {
+    // Assert that this is the entry block. If it isn't, then there is some
+    // code construct we don't recognise that deals with physregs across
+    // blocks.
+    assert(!State.first.isVirtual());
+    assert(&*InsertBB.getParent()->begin() == &InsertBB);
+  }
 
   // Create DBG_PHI for specified physreg.
-  auto Builder = BuildMI(TargetBB, TargetBB.getFirstNonPHI(), DebugLoc(),
+  auto Builder = BuildMI(InsertBB, InsertBB.getFirstNonPHI(), DebugLoc(),
                          TII.get(TargetOpcode::DBG_PHI));
   Builder.addReg(State.first);
   unsigned NewNum = getNewDebugInstrNum();


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D104520.352974.patch
Type: text/x-patch
Size: 3510 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210618/ee1dcb7e/attachment.bin>