[DebugInfo][X86] Improve X86 Optimize LEAs handling of debug values.

[andrewng]

Summary

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 make them available as static member functions of DIExpression.

[aprantl]

lib/Target/X86/X86OptimizeLEAs.cpp

267

No need to spell out \brief. LLVM uses autobrief in the doxygen settings.

544

this comment should go into the header instead.

555

This is for the backend to decide and discard. We shouldn't have any DWARF version specific code here. (E.g.: what if we generate CodeView debug info?)

After landing https://reviews.llvm.org/D31439, DwarfExpression can actually drop locations that can not be expressed in the requested DWARF version.

[andrewng]

lib/Target/X86/X86OptimizeLEAs.cpp

544

Do you mean the class definition?

555

OK, so I should just generate the DIExpression with the DW_OP_stack_value operation and the backend will discard the DBG_VALUE if the DWARF version is not compatible (in the case of DWARF debug info)?

[aprantl]

lib/Target/X86/X86OptimizeLEAs.cpp

555

Yes, please. Note that the backend cannot (yet) doe this, but I will hopefully be able to land this functionality this week.

[andrewng]

Removed DWARF version checks as suggested by Adrian (thanks) and updated comments.

[aprantl]

lib/Target/X86/X86OptimizeLEAs.cpp

549

Is AddrDispShift always > 0?

[aprantl]

lib/Target/X86/X86OptimizeLEAs.cpp

555

This change is finally in r300882.

[andrewng]

lib/Target/X86/X86OptimizeLEAs.cpp

549

I believe that AddrDispShift can be either positive or negative. I assume that it won't be zero as that would imply there were two "identical" LEAs.

[aprantl]

lib/Target/X86/X86OptimizeLEAs.cpp

549

Okay then I think there should be an assert(AddrDispShift != 0 && "...") here to make sure we change the ?: condition should this ever change in the future.

558

Are you sure you want the DW_OP_stack_value at position 0? Shouldn't it go either at the end of the expression or just before a DW_OP_llvm_fragment operation?

560

I think I would prefer std::next() over the +1.

[andrewng]

Sorry for the slight delay but only just got back from holiday.

lib/Target/X86/X86OptimizeLEAs.cpp

549

Could you please explain why the ?: condition would need to change if AddrDispShift == 0. I think it should work as it is and thus there is no need for the assertion.

558

Yes, I think it needs to be there otherwise any existing operations will be operating on the wrong value.

560

OK, I will change this.

[aprantl]

lib/Target/X86/X86OptimizeLEAs.cpp

558

Sorry, this was a rhetorical question. The DW_OP_stack_value operation always terminates the expression. It *must* come at the end (but before DW_OP_LLVM_fragement). You can factor out the code in Local.cpp from prependDIExpr() and move it into a static member function of DIExpression.

[andrewng]

lib/Target/X86/X86OptimizeLEAs.cpp

558

Thanks for the clarification. I re-read the spec more carefully and yes, I missed the "terminates the expression" part.

I've had a quick look at prependDIExpr. Could I use the offset to apply my AddrDispShift? Will the backend then sort out the specifics of generating a "correct" DWARF expression?

I think it is somewhat confusing that the DIExpression uses DWARF operations, but isn't strictly the equivalent of a DWARF expression. Or have I misunderstood?

[aprantl]

lib/Target/X86/X86OptimizeLEAs.cpp

558

I've had a quick look at prependDIExpr. Could I use the offset to apply my AddrDispShift? Will the backend then sort out the specifics of generating a "correct" DWARF expression?

I think so. Can you give me an example of how you imagine it failing, perhaps I'm not understanding your concern correctly?

I think it is somewhat confusing that the DIExpression uses DWARF operations, but isn't strictly the equivalent of a DWARF expression. Or have I misunderstood?

I recently tried to improve the documentation in LangRef.rst let me know if there are still things missing. The most painful difference that I'm aware of right now is the fact that DW_OP_plus/minus has an implicit operand.

[andrewng]

lib/Target/X86/X86OptimizeLEAs.cpp

558

I will experiment with prependDIExpr. It looks like it should do what I need. I'll get back to you if I have any issues.

I've looked at the DIExpression documentation and it looks good to me.

[andrewng]

As suggested by Adrian, I have made prependDIExpr a static member function of DIExpression (and also appendOffset used by prependDIExpr). This is then used to apply the LEA address shift to the replacement debug value.

[aprantl]

Thanks!

lib/Target/X86/X86OptimizeLEAs.cpp

 #include "X86Subtarget.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/CodeGen/LiveVariables.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineOperand.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/Passes.h"
+#include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/Function.h"
+#include "llvm/MC/MCContext.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetInstrInfo.h"
 
 using namespace llvm;
 
 #define DEBUG_TYPE "x86-optimize-LEAs"
 
private:
   /// \brief Find all LEA instructions in the basic block. Also, assign position
   /// numbers to all instructions in the basic block to speed up calculation of
   /// distance between them.
   void findLEAs(const MachineBasicBlock &MBB, MemOpMap &LEAs);
 
   /// \brief Removes redundant address calculations.
   bool removeRedundantAddrCalc(MemOpMap &LEAs);
 
+  /// \brief Replace debug value.
+  MachineInstr *replaceDebugValue(MachineInstr &MI, unsigned VReg,
+                                  int64_t AddrDispShift);
+
   /// \brief Removes LEAs which calculate similar addresses.
   bool removeRedundantLEAs(MemOpMap &LEAs);
 
   DenseMap<const MachineInstr *, unsigned> InstrPos;
 
   MachineRegisterInfo *MRI;
   const X86InstrInfo *TII;
   const X86RegisterInfo *TRI;
+  uint16_t DwarfVersion;
 
   static char ID;
 };
 char OptimizeLEAPass::ID = 0;
 }
 
 FunctionPass *llvm::createX86OptimizeLEAs() { return new OptimizeLEAPass(); }
 
for (auto I = MBB->begin(), E = MBB->end(); I != E;) {
     DEBUG(dbgs() << "OptimizeLEAs: Replaced by: "; MI.dump(););
 
     Changed = true;
   }
 
   return Changed;
 }
 
+// If possible replace debug value MI with a new debug value instruction using
+// register VReg with an appropriate offset and DIExpression to incorporate the
+// address displacement. Otherwise remove debug value.
+MachineInstr *OptimizeLEAPass::replaceDebugValue(MachineInstr &MI,
+                                                 unsigned VReg,
+                                                 int64_t AddrDispShift) {
+  const DIExpression *Expr = MI.getDebugExpression();
+
+  bool IsIndirect = MI.isIndirectDebugValue();
+  int64_t Offset = (IsIndirect ? MI.getOperand(1).getImm() : 0) + AddrDispShift;
+  if (!IsIndirect && (Offset != 0)) {
+    if (DwarfVersion < 4) {
+      // Can't do this with DWARF version less than 4 as we need support for
+      // DW_OP_stack_value, so remove.
+      MI.eraseFromParent();
+      return nullptr;
+    }
+
+    // If required create new DIExpression with DW_OP_stack_value prepended to
+    // existing DIExpression.
+    ArrayRef<uint64_t> OrigElements = Expr->getElements();
+    if (OrigElements.empty()) {
+      Expr = DIExpression::get(Expr->getContext(),
+                               { dwarf::DW_OP_stack_value });
+    } else if (OrigElements[0] != dwarf::DW_OP_stack_value) {
+      SmallVector<uint64_t, 8> Elements(OrigElements.size() + 1);
+      Elements[0] = dwarf::DW_OP_stack_value;
+      std::copy(OrigElements.begin(), OrigElements.end(),
+                Elements.begin() + 1);
+      Expr = DIExpression::get(Expr->getContext(), Elements);
+    }
+
+    IsIndirect = true;
+  }
+
+  // Replace DBG_VALUE instruction with modified version.
+  MachineBasicBlock *MBB = MI.getParent();
+  DebugLoc DL = MI.getDebugLoc();
+  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;
 
   // Loop over all entries in the table.
   for (auto &E : LEAs) {
     auto &List = E.second;
 
while (I1 != List.end()) {
         if (!isReplaceable(First, Last, AddrDispShift)) {
           ++I2;
           continue;
         }
 
         // 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()) {
+            // Attempt to 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 =
               X86II::getMemoryOperandNo(Desc.TSFlags) +
               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);
           if (Op.isImm())
             Op.setImm(Op.getImm() + AddrDispShift);
           else if (!Op.isJTI())
             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(););
 
         // By this moment, all of the Last LEA's uses must be replaced. So we
         // can freely remove it.
         assert(MRI->use_empty(LastVReg) &&
                "The LEA's def register must have no uses");
bool OptimizeLEAPass::runOnMachineFunction(MachineFunction &MF) {
   bool Changed = false;
 
   if (DisableX86LEAOpt || skipFunction(*MF.getFunction()))
     return false;
 
   MRI = &MF.getRegInfo();
   TII = MF.getSubtarget<X86Subtarget>().getInstrInfo();
   TRI = MF.getSubtarget<X86Subtarget>().getRegisterInfo();
+  DwarfVersion = MF.getContext().getDwarfVersion();
 
   // Process all basic blocks.
   for (auto &MBB : MF) {
     MemOpMap LEAs;
     InstrPos.clear();
 
     // Find all LEA instructions in basic block.
     findLEAs(MBB, LEAs);

test/CodeGen/X86/lea-opt-with-debug.mir

 # 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"
 
   %struct.A = type { i32, i32, i32 }
 
   @c = common local_unnamed_addr global %struct.A* null, align 8
   @a = common local_unnamed_addr global i32 0, align 4
   @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
   declare void @llvm.dbg.value(metadata, i64, metadata, metadata) #0
 
   attributes #0 = { nounwind readnone }
 
   !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: "")
   !2 = !{}
   !3 = !{!4}
   !4 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
   !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_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)
 
 ...
 ---
 name:            fn1
 alignment:       4
 exposesReturnsTwice: false
 legalized:       false
 regBankSelected: false
frameInfo:
   maxCallFrameSize: 0
   hasOpaqueSPAdjustment: false
   hasVAStart:      false
   hasMustTailInVarArgFunc: false
 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
-
-    %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
+    ; 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, 8, !12, !8, debug-location !17
+
+    %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
 
 ...