Place the lowered phi instruction(s) before the DEBUG_VALUE entry

[keith.walker.arm]

Summary

When a phi node is finally lowered to a machine instruction it is
important that the lowered "load" instruction is placed before the
associated DEBUG_VALUE entry describing the value loaded.

Renamed the existing SkipPHIsAndLabels to SkipPHIsLabelsAndDebug to
more fully describe that it also skips debug entries. The used the
"new" function SkipPHIsAndLabels when the debug information should not
be skipped when placing the lowered "load" instructions so that it is
placed before the debug entries.

[dblaikie]

Looks pretty reasonable - but, again, I'll punt to Adrian for final sign off due to better familiarity with the debug value/variable tracking code.

(minor wonder whether it'd be easy enough for you to take a quick look at other targets to see if they need a similar treatment to PPCEarlyReturn.cpp)

[aprantl]

Looks reasonable to me. Thanks!

include/llvm/CodeGen/MachineBasicBlock.h

463 ↗

(On Diff #68815)

or debug intrinsic. (?)
Sounds marginally better to my ears ;-)

llvm/trunk/include/llvm/CodeGen/MachineBasicBlock.h

public:
   /// Returns a pointer to the first instruction in this block that is not a
   /// PHINode instruction. When adding instructions to the beginning of the
   /// basic block, they should be added before the returned value, not before
   /// the first instruction, which might be PHI.
   /// Returns end() is there's no non-PHI instruction.
   iterator getFirstNonPHI();
 
   /// Return the first instruction in MBB after I that is not a PHI or a label.
-  /// This is the correct point to insert copies at the beginning of a basic
-  /// block.
+  /// This is the correct point to insert lowered copies at the beginning of a
+  /// basic block that must be before any debugging information.
   iterator SkipPHIsAndLabels(iterator I);
 
+  /// Return the first instruction in MBB after I that is not a PHI, label or
+  /// debug.  This is the correct point to insert copies at the beginning of a
+  /// basic block.
+  iterator SkipPHIsLabelsAndDebug(iterator I);
+
   /// Returns an iterator to the first terminator instruction of this basic
   /// block. If a terminator does not exist, it returns end().
   iterator getFirstTerminator();
   const_iterator getFirstTerminator() const {
     return const_cast<MachineBasicBlock *>(this)->getFirstTerminator();
   }
 
   /// Same getFirstTerminator but it ignores bundles and return an

llvm/trunk/lib/CodeGen/InlineSpiller.cpp

#endif
 
   // We are going to spill SrcVNI immediately after its def, so clear out
   // any later spills of the same value.
   eliminateRedundantSpills(SrcLI, SrcVNI);
 
   MachineBasicBlock *MBB = LIS.getMBBFromIndex(SrcVNI->def);
   MachineBasicBlock::iterator MII;
   if (SrcVNI->isPHIDef())
-    MII = MBB->SkipPHIsAndLabels(MBB->begin());
+    MII = MBB->SkipPHIsLabelsAndDebug(MBB->begin());
   else {
     MachineInstr *DefMI = LIS.getInstructionFromIndex(SrcVNI->def);
     assert(DefMI && "Defining instruction disappeared");
     MII = DefMI;
     ++MII;
   }
   // Insert spill without kill flag immediately after def.
   TII.storeRegToStackSlot(*MBB, MII, SrcReg, false, StackSlot,

llvm/trunk/lib/CodeGen/LiveDebugVariables.cpp

findInsertLocation(MachineBasicBlock *MBB, SlotIndex Idx,
   SlotIndex Start = LIS.getMBBStartIdx(MBB);
   Idx = Idx.getBaseIndex();
 
   // Try to find an insert location by going backwards from Idx.
   MachineInstr *MI;
   while (!(MI = LIS.getInstructionFromIndex(Idx))) {
     // We've reached the beginning of MBB.
     if (Idx == Start) {
-      MachineBasicBlock::iterator I = MBB->SkipPHIsAndLabels(MBB->begin());
+      MachineBasicBlock::iterator I = MBB->SkipPHIsLabelsAndDebug(MBB->begin());
       return I;
     }
     Idx = Idx.getPrevIndex();
   }
 
   // Don't insert anything after the first terminator, though.
   return MI->isTerminator() ? MBB->getFirstTerminator() :
                               std::next(MachineBasicBlock::iterator(MI));

llvm/trunk/lib/CodeGen/MachineBasicBlock.cpp

MachineBasicBlock::iterator MachineBasicBlock::getFirstNonPHI() {
   assert((I == E || !I->isInsideBundle()) &&
          "First non-phi MI cannot be inside a bundle!");
   return I;
 }
 
 MachineBasicBlock::iterator
 MachineBasicBlock::SkipPHIsAndLabels(MachineBasicBlock::iterator I) {
   iterator E = end();
+  while (I != E && (I->isPHI() || I->isPosition()))
+    ++I;
+  // FIXME: This needs to change if we wish to bundle labels
+  // inside the bundle.
+  assert((I == E || !I->isInsideBundle()) &&
+         "First non-phi / non-label instruction is inside a bundle!");
+  return I;
+}
+
+MachineBasicBlock::iterator
+MachineBasicBlock::SkipPHIsLabelsAndDebug(MachineBasicBlock::iterator I) {
+  iterator E = end();
   while (I != E && (I->isPHI() || I->isPosition() || I->isDebugValue()))
     ++I;
   // FIXME: This needs to change if we wish to bundle labels / dbg_values
   // inside the bundle.
   assert((I == E || !I->isInsideBundle()) &&
-         "First non-phi / non-label instruction is inside a bundle!");
+         "First non-phi / non-label / non-debug "
+         "instruction is inside a bundle!");
   return I;
 }
 
 MachineBasicBlock::iterator MachineBasicBlock::getFirstTerminator() {
   iterator B = begin(), E = end(), I = E;
   while (I != B && ((--I)->isTerminator() || I->isDebugValue()))
     ; /*noop */
   while (I != E && !I->isTerminator())

llvm/trunk/lib/CodeGen/PHIEliminationUtils.cpp

if (DefUsesInMBB.empty()) {
     ++InsertPoint;
   } else {
     // Insert the copy immediately after the last def/use.
     InsertPoint = MBB->end();
     while (!DefUsesInMBB.count(&*--InsertPoint)) {}
     ++InsertPoint;
   }
 
-  // Make sure the copy goes after any phi nodes however.
+  // Make sure the copy goes after any phi nodes but before
+  // any debug nodes.
   return MBB->SkipPHIsAndLabels(InsertPoint);
 }

llvm/trunk/lib/CodeGen/SplitKit.cpp

SlotIndex SplitEditor::leaveIntvAtTop(MachineBasicBlock &MBB) {
 
   VNInfo *ParentVNI = Edit->getParent().getVNInfoAt(Start);
   if (!ParentVNI) {
     DEBUG(dbgs() << ": not live\n");
     return Start;
   }
 
   VNInfo *VNI = defFromParent(0, ParentVNI, Start, MBB,
-                              MBB.SkipPHIsAndLabels(MBB.begin()));
+                              MBB.SkipPHIsLabelsAndDebug(MBB.begin()));
   RegAssign.insert(Start, VNI->def, OpenIdx);
   DEBUG(dump());
   return VNI->def;
 }
 
 void SplitEditor::overlapIntv(SlotIndex Start, SlotIndex End) {
   assert(OpenIdx && "openIntv not called before overlapIntv");
   const VNInfo *ParentVNI = Edit->getParent().getVNInfoAt(Start);

llvm/trunk/lib/Target/PowerPC/PPCEarlyReturn.cpp

struct PPCEarlyReturn : public MachineFunctionPass {
 
     const TargetInstrInfo *TII;
 
 protected:
     bool processBlock(MachineBasicBlock &ReturnMBB) {
       bool Changed = false;
 
       MachineBasicBlock::iterator I = ReturnMBB.begin();
-      I = ReturnMBB.SkipPHIsAndLabels(I);
+      I = ReturnMBB.SkipPHIsLabelsAndDebug(I);
 
       // The block must be essentially empty except for the blr.
       if (I == ReturnMBB.end() ||
           (I->getOpcode() != PPC::BLR && I->getOpcode() != PPC::BLR8) ||
           I != ReturnMBB.getLastNonDebugInstr())
         return Changed;
 
       SmallVector<MachineBasicBlock*, 8> PredToRemove;

llvm/trunk/test/CodeGen/AArch64/phi-dbg.ll

+; RUN: llc -O0 %s -mtriple=aarch64 -o - | FileCheck %s
+
+; Test that a DEBUG_VALUE node is create for variable c after the phi has been
+; converted to a ldr.    The DEBUG_VALUE must be *after* the ldr and not before it.
+
+; Created from the C code, compiled with -O0 -g and then passed through opt -mem2reg:
+;
+; int func(int a)
+; {
+;         int c = 1;
+;         if (a < 0 ) {
+;                 c = 12;
+;         }
+;         return c;
+; }
+;
+; Function Attrs: nounwind
+define i32 @func(i32) #0 !dbg !8 {
+  call void @llvm.dbg.value(metadata i32 %0, i64 0, metadata !12, metadata !13), !dbg !14
+  call void @llvm.dbg.value(metadata i32 1, i64 0, metadata !15, metadata !13), !dbg !16
+  %2 = icmp slt i32 %0, 0, !dbg !17
+  br i1 %2, label %3, label %4, !dbg !19
+
+; <label>:3:                                      ; preds = %1
+  call void @llvm.dbg.value(metadata i32 12, i64 0, metadata !15, metadata !13), !dbg !16
+  br label %4, !dbg !20
+
+; <label>:4:                                      ; preds = %3, %1
+  %.0 = phi i32 [ 12, %3 ], [ 1, %1 ]
+; CHECK: ldr     w[[REG:[0-9]+]], [sp, #8]
+; CHECK-NEXT: .Ltmp
+  call void @llvm.dbg.value(metadata i32 %.0, i64 0, metadata !15, metadata !13), !dbg !16
+; CHECK-NEXT:  //DEBUG_VALUE: func:c <- %W[[REG]]
+  %5 = add nsw i32 %.0, %0, !dbg !22
+  call void @llvm.dbg.value(metadata i32 %5, i64 0, metadata !15, metadata !13), !dbg !16
+  ret i32 %5, !dbg !23
+}
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.value(metadata, i64, metadata, metadata) #1
+
+attributes #0 = { nounwind }
+attributes #1 = { nounwind readnone }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6}
+!llvm.ident = !{!7}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "a.c", directory: "/tmp")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{i32 1, !"min_enum_size", i32 4}
+!7 = !{!"clang"}
+!8 = distinct !DISubprogram(name: "func", scope: !1, file: !1, line: 1, type: !9, isLocal: false, isDefinition: true, scopeLine: 2, flags: DIFlagPrototyped, isOptimized: false, unit: !0, variables: !2)
+!9 = !DISubroutineType(types: !10)
+!10 = !{!11, !11}
+!11 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
+!12 = !DILocalVariable(name: "a", arg: 1, scope: !8, file: !1, line: 1, type: !11)
+!13 = !DIExpression()
+!14 = !DILocation(line: 1, column: 14, scope: !8)
+!15 = !DILocalVariable(name: "c", scope: !8, file: !1, line: 3, type: !11)
+!16 = !DILocation(line: 3, column: 13, scope: !8)
+!17 = !DILocation(line: 4, column: 15, scope: !18)
+!18 = distinct !DILexicalBlock(scope: !8, file: !1, line: 4, column: 13)
+!19 = !DILocation(line: 4, column: 13, scope: !8)
+!20 = !DILocation(line: 6, column: 9, scope: !21)
+!21 = distinct !DILexicalBlock(scope: !18, file: !1, line: 4, column: 21)
+!22 = !DILocation(line: 7, column: 4, scope: !8)
+!23 = !DILocation(line: 8, column: 9, scope: !8)