[DebugInfo][X86] Improve X86 Optimize LEAs handling of debug values.
authorAndrew Ng <anng.sw@gmail.com>
Fri, 28 Apr 2017 08:44:30 +0000 (08:44 +0000)
committerAndrew Ng <anng.sw@gmail.com>
Fri, 28 Apr 2017 08:44:30 +0000 (08:44 +0000)
This is a follow up to the fix in r298360 to improve the handling of debug
values when redundant LEAs are removed. The fix in r298360 effectively
discarded the debug values. This patch now attempts to preserve the debug
values by using the DWARF DW_OP_stack_value operation via prependDIExpr.

Moved functions appendOffset and prependDIExpr from Local.cpp to
DebugInfoMetadata.cpp and made them available as static member functions of
DIExpression.

Differential Revision: https://reviews.llvm.org/D31604

llvm-svn: 301630

llvm/include/llvm/IR/DebugInfoMetadata.h
llvm/lib/IR/DebugInfoMetadata.cpp
llvm/lib/Target/X86/X86OptimizeLEAs.cpp
llvm/lib/Transforms/Utils/Local.cpp
llvm/test/CodeGen/X86/lea-opt-with-debug.mir

index 67f51cc..6f2d1cb 100644 (file)
@@ -56,6 +56,8 @@
 
 namespace llvm {
 
+class DIBuilder;
+
 template <typename T> class Optional;
 
 /// Holds a subclass of DINode.
@@ -2275,6 +2277,15 @@ public:
 
   /// Return whether this is a piece of an aggregate variable.
   bool isFragment() const { return getFragmentInfo().hasValue(); }
+
+  /// Append \p Ops with operations to apply the \p Offset.
+  static void appendOffset(SmallVectorImpl<uint64_t> &Ops, int64_t Offset);
+
+  /// Prepend \p DIExpr with a deref and offset operation and optionally turn it
+  /// into a stack value.
+  static DIExpression *prependDIExpr(DIBuilder &Builder, DIExpression *DIExpr,
+                                     bool Deref, int64_t Offset = 0,
+                                     bool StackValue = false);
 };
 
 /// Global variables.
index 3db9a3d..92f4853 100644 (file)
@@ -15,6 +15,7 @@
 #include "LLVMContextImpl.h"
 #include "MetadataImpl.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/IR/DIBuilder.h"
 #include "llvm/IR/Function.h"
 
 using namespace llvm;
@@ -660,6 +661,48 @@ DIExpression::getFragmentInfo(expr_op_iterator Start, expr_op_iterator End) {
   return None;
 }
 
+void DIExpression::appendOffset(SmallVectorImpl<uint64_t> &Ops,
+                                int64_t Offset) {
+  if (Offset > 0) {
+    Ops.push_back(dwarf::DW_OP_plus);
+    Ops.push_back(Offset);
+  } else if (Offset < 0) {
+    Ops.push_back(dwarf::DW_OP_minus);
+    Ops.push_back(-Offset);
+  }
+}
+
+DIExpression *
+DIExpression::prependDIExpr(DIBuilder &Builder, DIExpression *DIExpr,
+                            bool Deref, int64_t Offset,
+                            bool StackValue) {
+  if (!Deref && !Offset && !StackValue)
+    return DIExpr;
+
+  SmallVector<uint64_t, 8> Ops;
+  appendOffset(Ops, Offset);
+  if (Deref)
+    Ops.push_back(dwarf::DW_OP_deref);
+  if (DIExpr)
+    for (auto Op : DIExpr->expr_ops()) {
+      // A DW_OP_stack_value comes at the end, but before a DW_OP_LLVM_fragment.
+      if (StackValue) {
+        if (Op.getOp() == dwarf::DW_OP_stack_value)
+          StackValue = false;
+        else if (Op.getOp() == dwarf::DW_OP_LLVM_fragment) {
+          Ops.push_back(dwarf::DW_OP_stack_value);
+          StackValue = false;
+        }
+      }
+      Ops.push_back(Op.getOp());
+      for (unsigned I = 0; I < Op.getNumArgs(); ++I)
+        Ops.push_back(Op.getArg(I));
+    }
+  if (StackValue)
+    Ops.push_back(dwarf::DW_OP_stack_value);
+  return Builder.createExpression(Ops);
+}
+
 bool DIExpression::isConstant() const {
   // Recognize DW_OP_constu C DW_OP_stack_value (DW_OP_LLVM_fragment Len Ofs)?.
   if (getNumElements() != 3 && getNumElements() != 6)
index debb192..28c0757 100644 (file)
@@ -27,6 +27,8 @@
 #include "llvm/CodeGen/MachineOperand.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/Passes.h"
+#include "llvm/IR/DebugInfoMetadata.h"
+#include "llvm/IR/DIBuilder.h"
 #include "llvm/IR/Function.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
@@ -221,6 +223,8 @@ public:
 
   StringRef getPassName() const override { return "X86 LEA Optimize"; }
 
+  bool doInitialization(Module &M) override;
+
   /// \brief Loop over all of the basic blocks, replacing address
   /// calculations in load and store instructions, if it's already
   /// been calculated by LEA. Also, remove redundant LEAs.
@@ -262,6 +266,12 @@ private:
   /// \brief Removes redundant address calculations.
   bool removeRedundantAddrCalc(MemOpMap &LEAs);
 
+  /// Replace debug value MI with a new debug value instruction using register
+  /// VReg with an appropriate offset and DIExpression to incorporate the
+  /// address displacement AddrDispShift. Return new debug value instruction.
+  MachineInstr *replaceDebugValue(MachineInstr &MI, unsigned VReg,
+                                  int64_t AddrDispShift);
+
   /// \brief Removes LEAs which calculate similar addresses.
   bool removeRedundantLEAs(MemOpMap &LEAs);
 
@@ -270,6 +280,7 @@ private:
   MachineRegisterInfo *MRI;
   const X86InstrInfo *TII;
   const X86RegisterInfo *TRI;
+  Module *TheModule;
 
   static char ID;
 };
@@ -532,6 +543,26 @@ bool OptimizeLEAPass::removeRedundantAddrCalc(MemOpMap &LEAs) {
   return Changed;
 }
 
+MachineInstr *OptimizeLEAPass::replaceDebugValue(MachineInstr &MI,
+                                                 unsigned VReg,
+                                                 int64_t AddrDispShift) {
+  DIExpression *Expr = const_cast<DIExpression *>(MI.getDebugExpression());
+
+  if (AddrDispShift != 0) {
+    DIBuilder DIB(*TheModule);
+    Expr = DIExpression::prependDIExpr(DIB, Expr, false, AddrDispShift, true);
+  }
+
+  // Replace DBG_VALUE instruction with modified version.
+  MachineBasicBlock *MBB = MI.getParent();
+  DebugLoc DL = MI.getDebugLoc();
+  bool IsIndirect = MI.isIndirectDebugValue();
+  int64_t Offset = IsIndirect ? MI.getOperand(1).getImm() : 0;
+  const MDNode *Var = MI.getDebugVariable();
+  return BuildMI(*MBB, MBB->erase(&MI), DL, TII->get(TargetOpcode::DBG_VALUE),
+                 IsIndirect, VReg, Offset, Var, Expr);
+}
+
 // Try to find similar LEAs in the list and replace one with another.
 bool OptimizeLEAPass::removeRedundantLEAs(MemOpMap &LEAs) {
   bool Changed = false;
@@ -563,13 +594,21 @@ bool OptimizeLEAPass::removeRedundantLEAs(MemOpMap &LEAs) {
         // Loop over all uses of the Last LEA and update their operands. Note
         // that the correctness of this has already been checked in the
         // isReplaceable function.
+        unsigned FirstVReg = First.getOperand(0).getReg();
         unsigned LastVReg = Last.getOperand(0).getReg();
-        for (auto UI = MRI->use_nodbg_begin(LastVReg),
-                  UE = MRI->use_nodbg_end();
+        for (auto UI = MRI->use_begin(LastVReg), UE = MRI->use_end();
              UI != UE;) {
           MachineOperand &MO = *UI++;
           MachineInstr &MI = *MO.getParent();
 
+          if (MI.isDebugValue()) {
+            // Replace DBG_VALUE instruction with modified version using the
+            // register from the replacing LEA and the address displacement
+            // between the LEA instructions.
+            replaceDebugValue(MI, FirstVReg, AddrDispShift);
+            continue;
+          }
+
           // Get the number of the first memory operand.
           const MCInstrDesc &Desc = MI.getDesc();
           int MemOpNo =
@@ -577,7 +616,7 @@ bool OptimizeLEAPass::removeRedundantLEAs(MemOpMap &LEAs) {
               X86II::getOperandBias(Desc);
 
           // Update address base.
-          MO.setReg(First.getOperand(0).getReg());
+          MO.setReg(FirstVReg);
 
           // Update address disp.
           MachineOperand &Op = MI.getOperand(MemOpNo + X86::AddrDisp);
@@ -587,11 +626,8 @@ bool OptimizeLEAPass::removeRedundantLEAs(MemOpMap &LEAs) {
             Op.setOffset(Op.getOffset() + AddrDispShift);
         }
 
-        // Mark debug values referring to Last LEA as undefined.
-        MRI->markUsesInDebugValueAsUndef(LastVReg);
-
         // Since we can possibly extend register lifetime, clear kill flags.
-        MRI->clearKillFlags(First.getOperand(0).getReg());
+        MRI->clearKillFlags(FirstVReg);
 
         ++NumRedundantLEAs;
         DEBUG(dbgs() << "OptimizeLEAs: Remove redundant LEA: "; Last.dump(););
@@ -614,6 +650,11 @@ bool OptimizeLEAPass::removeRedundantLEAs(MemOpMap &LEAs) {
   return Changed;
 }
 
+bool OptimizeLEAPass::doInitialization(Module &M) {
+  TheModule = &M;
+  return false;
+}
+
 bool OptimizeLEAPass::runOnMachineFunction(MachineFunction &MF) {
   bool Changed = false;
 
index d3002c5..1b643c5 100644 (file)
@@ -1259,50 +1259,8 @@ void llvm::findDbgValues(SmallVectorImpl<DbgValueInst *> &DbgValues, Value *V) {
           DbgValues.push_back(DVI);
 }
 
-static void appendOffset(SmallVectorImpl<uint64_t> &Ops, int64_t Offset) {
-  if (Offset > 0) {
-    Ops.push_back(dwarf::DW_OP_plus);
-    Ops.push_back(Offset);
-  } else if (Offset < 0) {
-    Ops.push_back(dwarf::DW_OP_minus);
-    Ops.push_back(-Offset);
-  }
-}
-
 enum { WithStackValue = true };
 
-/// Prepend \p DIExpr with a deref and offset operation and optionally turn it
-/// into a stack value.
-static DIExpression *prependDIExpr(DIBuilder &Builder, DIExpression *DIExpr,
-                                   bool Deref, int64_t Offset = 0,
-                                   bool StackValue = false) {
-  if (!Deref && !Offset && !StackValue)
-    return DIExpr;
-
-  SmallVector<uint64_t, 8> Ops;
-  appendOffset(Ops, Offset);
-  if (Deref)
-    Ops.push_back(dwarf::DW_OP_deref);
-  if (DIExpr)
-    for (auto Op : DIExpr->expr_ops()) {
-      // A DW_OP_stack_value comes at the end, but before a DW_OP_LLVM_fragment.
-      if (StackValue) {
-        if (Op.getOp() == dwarf::DW_OP_stack_value)
-          StackValue = false;
-        else if (Op.getOp() == dwarf::DW_OP_LLVM_fragment) {
-          Ops.push_back(dwarf::DW_OP_stack_value);
-          StackValue = false;
-        }
-      }
-      Ops.push_back(Op.getOp());
-      for (unsigned I = 0; I < Op.getNumArgs(); ++I)
-        Ops.push_back(Op.getArg(I));
-    }
-  if (StackValue)
-    Ops.push_back(dwarf::DW_OP_stack_value);
-  return Builder.createExpression(Ops);
-}
-
 bool llvm::replaceDbgDeclare(Value *Address, Value *NewAddress,
                              Instruction *InsertBefore, DIBuilder &Builder,
                              bool Deref, int Offset) {
@@ -1314,7 +1272,7 @@ bool llvm::replaceDbgDeclare(Value *Address, Value *NewAddress,
   auto *DIExpr = DDI->getExpression();
   assert(DIVar && "Missing variable");
 
-  DIExpr = prependDIExpr(Builder, DIExpr, Deref, Offset);
+  DIExpr = DIExpression::prependDIExpr(Builder, DIExpr, Deref, Offset);
 
   // Insert llvm.dbg.declare immediately after the original alloca, and remove
   // old llvm.dbg.declare.
@@ -1348,7 +1306,7 @@ static void replaceOneDbgValueForAlloca(DbgValueInst *DVI, Value *NewAddress,
   if (Offset) {
     SmallVector<uint64_t, 4> Ops;
     Ops.push_back(dwarf::DW_OP_deref);
-    appendOffset(Ops, Offset);
+    DIExpression::appendOffset(Ops, Offset);
     Ops.append(DIExpr->elements_begin() + 1, DIExpr->elements_end());
     DIExpr = Builder.createExpression(Ops);
   }
@@ -1398,8 +1356,9 @@ void llvm::salvageDebugInfo(Instruction &I) {
         auto *DIExpr = DVI->getExpression();
         DIBuilder DIB(M, /*AllowUnresolved*/ false);
         // GEP offsets are i32 and thus always fit into an int64_t.
-        DIExpr = prependDIExpr(DIB, DIExpr, NoDeref, Offset.getSExtValue(),
-                               WithStackValue);
+        DIExpr = DIExpression::prependDIExpr(DIB, DIExpr, NoDeref,
+                                             Offset.getSExtValue(),
+                                             WithStackValue);
         DVI->setOperand(0, MDWrap(I.getOperand(0)));
         DVI->setOperand(3, MetadataAsValue::get(I.getContext(), DIExpr));
         DEBUG(dbgs() << "SALVAGE: " << *DVI << '\n');
@@ -1411,7 +1370,7 @@ void llvm::salvageDebugInfo(Instruction &I) {
       // Rewrite the load into DW_OP_deref.
       auto *DIExpr = DVI->getExpression();
       DIBuilder DIB(M, /*AllowUnresolved*/ false);
-      DIExpr = prependDIExpr(DIB, DIExpr, WithDeref);
+      DIExpr = DIExpression::prependDIExpr(DIB, DIExpr, WithDeref);
       DVI->setOperand(0, MDWrap(I.getOperand(0)));
       DVI->setOperand(3, MetadataAsValue::get(I.getContext(), DIExpr));
       DEBUG(dbgs() << "SALVAGE:  " << *DVI << '\n');
index ebf86ff..0a47770 100644 (file)
@@ -1,7 +1,8 @@
-# RUN: llc -mtriple=x86_64-unknown-unknown -start-after peephole-opt -stop-before detect-dead-lanes -o - %s | FileCheck %s
+# RUN: llc -mtriple=x86_64-unknown-unknown -start-after=peephole-opt -stop-before=detect-dead-lanes -o - %s | FileCheck %s
 
-# Test that pass optimize LEA can remove a redundant LEA even when it is also
-# used by a DBG_VALUE.
+# Test that the optimize LEA pass can remove a redundant LEA even when it is
+# also used by a DBG_VALUE. Check that the uses of the replaced LEA are updated
+# correctly.
 
 --- |
   target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
   @d = common local_unnamed_addr global i32 0, align 4
   @b = common local_unnamed_addr global i32 0, align 4
 
-  define i32 @fn1() local_unnamed_addr !dbg !8 {
-    %1 = load %struct.A*, %struct.A** @c, align 8, !dbg !13
-    %2 = load i32, i32* @a, align 4, !dbg !13
-    %3 = sext i32 %2 to i64, !dbg !13
-    %4 = getelementptr inbounds %struct.A, %struct.A* %1, i64 %3, !dbg !13
-    %5 = ptrtoint %struct.A* %4 to i64, !dbg !13
-    %6 = trunc i64 %5 to i32, !dbg !13
-    store i32 %6, i32* @d, align 4, !dbg !13
-    %7 = getelementptr inbounds %struct.A, %struct.A* %1, i64 %3, i32 2, !dbg !14
-    tail call void @llvm.dbg.value(metadata i32* %7, i64 0, metadata !11, metadata !15), !dbg !16
-    br label %8, !dbg !17
+  define i32 @fn1() local_unnamed_addr !dbg !9 {
+    %1 = load %struct.A*, %struct.A** @c, align 8, !dbg !14
+    %2 = load i32, i32* @a, align 4, !dbg !14
+    %3 = sext i32 %2 to i64, !dbg !14
+    %4 = getelementptr inbounds %struct.A, %struct.A* %1, i64 %3, !dbg !14
+    %5 = ptrtoint %struct.A* %4 to i64, !dbg !14
+    %6 = trunc i64 %5 to i32, !dbg !14
+    store i32 %6, i32* @d, align 4, !dbg !14
+    %7 = getelementptr inbounds %struct.A, %struct.A* %1, i64 %3, i32 2, !dbg !15
+    tail call void @llvm.dbg.value(metadata i32* %7, i64 0, metadata !12, metadata !16), !dbg !17
+    br label %8, !dbg !18
 
   ; <label>:8:                                      ; preds = %8, %0
-    %9 = load i32, i32* %7, align 4, !dbg !18
-    store i32 %9, i32* @d, align 4, !dbg !18
-    br label %8, !dbg !19
+    %9 = load i32, i32* %7, align 4, !dbg !19
+    store i32 %9, i32* @d, align 4, !dbg !19
+    br label %8, !dbg !20
   }
 
   ; Function Attrs: nounwind readnone
@@ -38,6 +39,7 @@
 
   !llvm.dbg.cu = !{!0}
   !llvm.module.flags = !{!5, !6, !7}
+  !misc = !{!8}
 
   !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !3, globals: !2)
   !1 = !DIFile(filename: "test.c", directory: "")
   !5 = !{i32 2, !"Dwarf Version", i32 4}
   !6 = !{i32 2, !"Debug Info Version", i32 3}
   !7 = !{i32 1, !"PIC Level", i32 2}
-  !8 = distinct !DISubprogram(name: "fn1", scope: !1, file: !1, line: 7, type: !9, isLocal: false, isDefinition: true, scopeLine: 7, isOptimized: true, unit: !0, variables: !10)
-  !9 = !DISubroutineType(types: !3)
-  !10 = !{!11}
-  !11 = !DILocalVariable(name: "e", scope: !8, file: !1, line: 8, type: !12)
-  !12 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !4, size: 64)
-  !13 = !DILocation(line: 9, scope: !8)
-  !14 = !DILocation(line: 10, scope: !8)
-  !15 = !DIExpression()
-  !16 = !DILocation(line: 8, scope: !8)
-  !17 = !DILocation(line: 11, scope: !8)
-  !18 = !DILocation(line: 13, scope: !8)
-  !19 = !DILocation(line: 14, scope: !8)
+  !8 = !DIExpression(DW_OP_plus, 8, DW_OP_stack_value)
+  !9 = distinct !DISubprogram(name: "fn1", scope: !1, file: !1, line: 7, type: !10, isLocal: false, isDefinition: true, scopeLine: 7, isOptimized: true, unit: !0, variables: !11)
+  !10 = !DISubroutineType(types: !3)
+  !11 = !{!12}
+  !12 = !DILocalVariable(name: "e", scope: !9, file: !1, line: 8, type: !13)
+  !13 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !4, size: 64)
+  !14 = !DILocation(line: 9, scope: !9)
+  !15 = !DILocation(line: 10, scope: !9)
+  !16 = !DIExpression()
+  !17 = !DILocation(line: 8, scope: !9)
+  !18 = !DILocation(line: 11, scope: !9)
+  !19 = !DILocation(line: 13, scope: !9)
+  !20 = !DILocation(line: 14, scope: !9)
 
 ...
 ---
@@ -95,28 +98,28 @@ body:             |
   bb.0 (%ir-block.0):
     successors: %bb.1(0x80000000)
 
-    ; CHECK: %3 = LEA64r %2, 2, %2, 0, _, debug-location !13
-    ; CHECK-NEXT: %4 = LEA64r %1, 4, %3, 0, _, debug-location !13
-    ; CHECK-NOT: %0 = LEA64r %1, 4, %3, 8, _, debug-location !14
-    ; CHECK: DBG_VALUE debug-use _, debug-use _, !11, !15, debug-location !16
+    ; CHECK: %3 = LEA64r %2, 2, %2, 0, _, debug-location !14
+    ; CHECK-NEXT: %4 = LEA64r %1, 4, %3, 0, _, debug-location !14
+    ; CHECK-NOT: %0 = LEA64r %1, 4, %3, 8, _, debug-location !15
+    ; CHECK: DBG_VALUE debug-use %4, debug-use _, !12, !8, debug-location !17
 
-    %1 = MOV64rm %rip, 1, _, @c, _, debug-location !13 :: (dereferenceable load 8 from @c)
-    %2 = MOVSX64rm32 %rip, 1, _, @a, _, debug-location !13 :: (dereferenceable load 4 from @a)
-    %3 = LEA64r %2, 2, %2, 0, _, debug-location !13
-    %4 = LEA64r %1, 4, %3, 0, _, debug-location !13
-    %5 = COPY %4.sub_32bit, debug-location !13
-    MOV32mr %rip, 1, _, @d, _, killed %5, debug-location !13 :: (store 4 into @d)
-    %0 = LEA64r %1, 4, %3, 8, _, debug-location !14
-    DBG_VALUE debug-use %0, debug-use _, !11, !15, debug-location !16
+    %1 = MOV64rm %rip, 1, _, @c, _, debug-location !14 :: (dereferenceable load 8 from @c)
+    %2 = MOVSX64rm32 %rip, 1, _, @a, _, debug-location !14 :: (dereferenceable load 4 from @a)
+    %3 = LEA64r %2, 2, %2, 0, _, debug-location !14
+    %4 = LEA64r %1, 4, %3, 0, _, debug-location !14
+    %5 = COPY %4.sub_32bit, debug-location !14
+    MOV32mr %rip, 1, _, @d, _, killed %5, debug-location !14 :: (store 4 into @d)
+    %0 = LEA64r %1, 4, %3, 8, _, debug-location !15
+    DBG_VALUE debug-use %0, debug-use _, !12, !16, debug-location !17
 
     ; CHECK-LABEL: bb.1 (%ir-block.8):
-    ; CHECK: %6 = MOV32rm %4, 1, _, 8, _, debug-location !18 :: (load 4 from %ir.7)
+    ; CHECK: %6 = MOV32rm %4, 1, _, 8, _, debug-location !19 :: (load 4 from %ir.7)
 
   bb.1 (%ir-block.8):
     successors: %bb.1(0x80000000)
 
-    %6 = MOV32rm %0, 1, _, 0, _, debug-location !18 :: (load 4 from %ir.7)
-    MOV32mr %rip, 1, _, @d, _, killed %6, debug-location !18 :: (store 4 into @d)
-    JMP_1 %bb.1, debug-location !19
+    %6 = MOV32rm %0, 1, _, 0, _, debug-location !19 :: (load 4 from %ir.7)
+    MOV32mr %rip, 1, _, @d, _, killed %6, debug-location !19 :: (store 4 into @d)
+    JMP_1 %bb.1, debug-location !20
 
 ...