[PATCH] D54712: [WIP][DebugInfo] Fix SelectionDAGs placement of phi-reading dbg.values

[Jeremy Morse via Phabricator via llvm-commits]

jmorse created this revision.
Herald added subscribers: llvm-commits, MatzeB.

This is a work-in-progress related to PR38754 [0], put up for visibility rather than review.

The test modified in this patch contained some FIXMEs added by the author when he noticed that DBG_VALUEs were being placed out of order. This patch fixes two of the FIXME'd placements, leaves one that isn't fixed, and marks up another incorrect DBG_VALUE placement that was pre-existing.

The cause is an interaction between source-IR-ordering and phi nodes: in the relevant BB, three phi nodes are read, which SelectionDAG pre-emptively allocates VRegs for. These are then brought into the DAG through CopyFromReg nodes. However, when emitted to a machine BB CopyFromReg doesn't create any instruction, and a nullptr entry is put into the IR-Location -> MachineInstr map. EmitSchedule can't place the DBG_VALUE there, so puts it one insn later than it would have otherwise.

The (rough) patch causes ProcessSourceNode to ignore SDNodes that didn't produce an instruction. That means subsequent SDNodes producing MachineInstrs get entered as valid locations in the IR-Location -> MachineInstr map, and EmitSchedule can emit DBG_VALUEs at the intended location.

The edge case is where an IR-Location / order-number doesn't have any MachineInstrs emitted at all: then the logic in EmitSchedule places DBG_VALUEs as early as it can, which is no different to how it behaves at the moment.

[0] https://bugs.llvm.org/show_bug.cgi?id=38754


Repository:
  rL LLVM

https://reviews.llvm.org/D54712

Files:
  lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
  test/DebugInfo/X86/sdag-dbgvalue-phi-use-2.ll


Index: test/DebugInfo/X86/sdag-dbgvalue-phi-use-2.ll
===================================================================
--- test/DebugInfo/X86/sdag-dbgvalue-phi-use-2.ll
+++ test/DebugInfo/X86/sdag-dbgvalue-phi-use-2.ll
@@ -44,14 +44,17 @@
 ; CHECK-NEXT: [[REG4:%[0-9]+]]:gr32 = PHI
 ; CHECK-NEXT: DBG_VALUE [[REG3]], $noreg, !16
 ; CHECK-NEXT: DBG_VALUE 555, $noreg, !17
-; XXX: Shouldn't the following DBG_VALUE be placed after the add (ADD32rr).
+; CHECK-NEXT: [[ADDREG:%[0-9]+]]:gr32 = nuw nsw ADD32rr
 ; CHECK-NEXT: DBG_VALUE [[REG2]], $noreg, !17
-; CHECK-NEXT: ADD32rr
-; XXX: Shouldn't the following DBG_VALUE be placed after the mul (LEA etc).
+; CHECK:      [[MULREG:%[0-9]+]]:gr32 = LEA64_32r
 ; CHECK-NEXT: DBG_VALUE 777, $noreg, !17
-; CHECK:      INC32r
+; XXX: The following DBG_VALUE should have stayed below the INC32r
+; CHECK-NEXT: DBG_VALUE [[MULREG]], $noreg, !16
+; CHECK-NEXT: [[INCREG:%[0-9]+]]:gr32 = nuw nsw INC32r
+; CHECK-NEXT: DBG_VALUE [[ADDREG]], $noreg, !15
+; CHECK-NEXT: DBG_VALUE [[INCREG]], $noreg, !17
 ; XXX: Shouldn't the following DBG_VALUE be placed after the icmp (the non-dead implicit def of $eflags)
-; CHECK:      DBG_VALUE [[REG4]]
+; CHECK-NEXT: DBG_VALUE [[REG4]]
 ; CHECK-NEXT: implicit-def $eflags
   %u.023 = phi i32 [ 0, %for.body.lr.ph ], [ %inc, %for.body ]
   %y.022 = phi i32 [ 13, %for.body.lr.ph ], [ %mul, %for.body ]
Index: lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
+++ lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
@@ -745,7 +745,7 @@
                   SmallVectorImpl<std::pair<unsigned, MachineInstr*> > &Orders,
                   SmallSet<unsigned, 8> &Seen) {
   unsigned Order = N->getIROrder();
-  if (!Order || !Seen.insert(Order).second) {
+  if (!Order || Seen.count(Order) != 0) {
     // Process any valid SDDbgValues even if node does not have any order
     // assigned.
     ProcessSDDbgValues(N, DAG, Emitter, Orders, VRBaseMap, 0);
@@ -759,10 +759,11 @@
       // BB->back().isPHI() test will not fire when we want it to.
       std::prev(IP)->isPHI()) {
     // Did not insert any instruction.
-    Orders.push_back({Order, (MachineInstr *)nullptr});
+    //Orders.push_back({Order, (MachineInstr *)nullptr});
     return;
   }
 
+  Seen.insert(Order);
   Orders.push_back({Order, &*std::prev(IP)});
   ProcessSDDbgValues(N, DAG, Emitter, Orders, VRBaseMap, Order);
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D54712.174634.patch
Type: text/x-patch
Size: 2479 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181119/a3121333/attachment.bin>