[DebugInfo][MachineCSE] Don't try to copy-propagate debuginfo for every COPY seen

[jmorse]

Summary

This patch alters the debug-info salvaging behaviour of MachineCSE, and adds a test for its behaviour (only one opaque test is currently sensitive to MachineCSEs DI salvaging).

In the loop in MachineCSE::PerformTrivialCopyPropagation (see patch), for an arbitrary input instruction, MachineCSE tries to copy-propagate vreg reading operands to expose more CSE opportunities. This is great -- but the current call to changeDebugValuesDefReg then unconditionally rewrites the DBG_VALUE users of the COPY that was propagated. This means that any COPY instruction whose result is the subject of CSE will have its DBG_VALUEs copy-propagated too, even if the COPY lives on, which is a bit overkill. I believe this propagation is only necessary if the COPY is to be deleted -- hence in this patch, the DBG_VALUE updating is moved to only occur if the COPY is deleted.

While we're here, alter changeDebugValuesDefReg to find DBG_VALUEs by enumerating vreg users, which catches DBG_VALUEs that don't immediately follow the COPY. MachineCSE is the only remaining user of this function (assuming D56265 makes it in).

I've added a test that stimulates both behaviours -- first where a COPY is deleted (and thus all DBG_VALUE users must update) and one where it doesn't.

A clang-3.4 build with this patch ticks up a fractional amount, and the covered scope bytes ticks down a fraction too. The ticking down is likely because vreg liveness interacting with DBG_VALUEs is still an open problem.

[aprantl]

lib/CodeGen/MachineCSE.cpp

197

Can you leave an explanatory comment here that will prevent somebody coming in later and moving it back to old location?

[vsk]

Looks reasonable to me.

lib/CodeGen/MachineInstr.cpp

2127

Reuse 'DefReg = getOperand(0).getReg()'?

[jmorse]

Rebase, update jump-insts in MIR test to latest version, address review comments (cheers).

[vsk]

Lgtm, thanks.

[jmorse]

Committed in https://reviews.llvm.org/rL370648

During which I failed to flush the buffer where I'd written a commit message and given the correct phabricator review link -- my bad :(.

include/llvm/CodeGen/MachineInstr.h

   }
 
   /// Add all implicit def and use operands to this instruction.
   void addImplicitDefUseOperands(MachineFunction &MF);
 
   /// Scan instructions following MI and collect any matching DBG_VALUEs.
   void collectDebugValues(SmallVectorImpl<MachineInstr *> &DbgValues);
 
-  /// Find all DBG_VALUEs immediately following this instruction that point
-  /// to a register def in this instruction and point them to \p Reg instead.
+  /// Find all DBG_VALUEs that point to the register def in this instruction
+  /// and point them to \p Reg instead.
   void changeDebugValuesDefReg(unsigned Reg);
 
 private:
   /// If this instruction is embedded into a MachineFunction, return the
   /// MachineRegisterInfo object for the current function, otherwise
   /// return null.
   MachineRegisterInfo *getRegInfo();
 

lib/CodeGen/MachineCSE.cpp

for (MachineOperand &MO : MI->operands()) {
     // class given a super-reg class and subreg index.
     if (DefMI->getOperand(1).getSubReg())
       continue;
     if (!MRI->constrainRegAttrs(SrcReg, Reg))
       continue;
     LLVM_DEBUG(dbgs() << "Coalescing: " << *DefMI);
     LLVM_DEBUG(dbgs() << "***     to: " << *MI);
 
-    // Update matching debug values.
-    DefMI->changeDebugValuesDefReg(SrcReg);
-
     // Propagate SrcReg of copies to MI.
     MO.setReg(SrcReg);
     MRI->clearKillFlags(SrcReg);
     // Coalesce single use copies.
     if (OnlyOneUse) {
+      // If (and only if) we've eliminated all uses of the copy, also
+      // copy-propagate to any debug-users of MI, or they'll be left using
+      // an undefined value.
+      DefMI->changeDebugValuesDefReg(SrcReg);
+
       DefMI->eraseFromParent();
       ++NumCoalesces;
     }
     Changed = true;
   }
 
   return Changed;
 }

lib/CodeGen/MachineInstr.cpp

if (DI->getOperand(0).isReg() &&
         DI->getOperand(0).getReg() == MI.getOperand(0).getReg())
       DbgValues.push_back(&*DI);
   }
 }
 
 void MachineInstr::changeDebugValuesDefReg(unsigned Reg) {
   // Collect matching debug values.
   SmallVector<MachineInstr *, 2> DbgValues;
-  collectDebugValues(DbgValues);
+
+  if (!getOperand(0).isReg())
+    return;
+
+  unsigned DefReg = getOperand(0).getReg();
+  auto *MRI = getRegInfo();
+  for (auto &MO : MRI->use_operands(DefReg)) {
+    auto *DI = MO.getParent();
+    if (!DI->isDebugValue())
+      continue;
+    if (DI->getOperand(0).isReg() &&
+        DI->getOperand(0).getReg() == DefReg){
+      DbgValues.push_back(DI);
+    }
+  }
 
   // Propagate Reg to debug value instructions.
   for (auto *DBI : DbgValues)
     DBI->getOperand(0).setReg(Reg);
 }
 
 using MMOList = SmallVector<const MachineMemOperand *, 2>;
 

test/DebugInfo/MIR/X86/machine-cse.mir

+# RUN: llc %s -o - -run-pass=machine-cse -mtriple=x86_64-- | FileCheck %s
+#
+# This test examines machine-cse's behaviour when dealing with copy propagation,
+# the code for which is lifted from test/CodeGen/X86/machine-cse.ll. There are
+# two (MIR) function that have SHL/LEA instructions CSE'd in the bb.1.bb1 block.
+# They both depend on the COPY of a vreg to %100 in the entry block.
+#
+# In the first (@t) there's only one use of %100, and that gets CSE'd away. The
+# corresponding COPY is deleted, and all DBG_VALUEs that refer to it must be
+# updated.
+#
+# In the second (@u) there are two uses of %100, one of which isn't deleted. The
+# DBG_VALUE users of %100 don't need to be updated -- test that they're not.
+--- |
+  target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-unknown-unknown"
+  
+  %struct.s2 = type { i32, i8*, i8*, [256 x %struct.s1*], [8 x i32], i64, i8*, i32, i64, i64, i32, %struct.s3*, %struct.s3*, [49 x i64] }
+  %struct.s1 = type { %ptr, %ptr }
+  %ptr = type { i8* }
+  %struct.s3 = type { %struct.s3*, %struct.s3*, i32, i32, i32 }
+  
+  ; Function Attrs: nounwind readnone speculatable
+  declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+  
+  define fastcc i8* @t(i32 %base) !dbg !3 {
+  entry:
+    %0 = zext i32 %base to i64
+    %1 = getelementptr inbounds %struct.s2, %struct.s2* null, i64 %0
+    br i1 undef, label %bb1, label %bb2
+  
+  bb1:                                              ; preds = %entry
+    %2 = getelementptr inbounds %struct.s2, %struct.s2* null, i64 %0, i32 0
+    call void @llvm.dbg.value(metadata i32* %2, metadata !4, metadata !DIExpression()), !dbg !7
+    call void @bar(i32* %2)
+    unreachable
+  
+  bb2:                                              ; preds = %entry
+    %3 = ptrtoint %struct.s2* %1 to i64
+    call void @baz(i64 %3)
+    unreachable
+  }
+  
+  ; This is a stub replicating bb structure of @t
+  define fastcc i8* @u(i32 %base) !dbg !33 {
+  entry:
+    br i1 undef, label %bb1, label %bb2
+  
+  bb1:                                              ; preds = %entry
+    unreachable
+  
+  bb2:                                              ; preds = %entry
+    unreachable
+  }
+ 
+
+  declare void @bar(i32*)
+  
+  declare void @baz(i64)
+  
+  declare i8* @foo(%struct.s2*)
+  
+  ; Function Attrs: nounwind
+  declare void @llvm.stackprotector(i8*, i8**) #1
+  
+  attributes #0 = { nounwind readnone speculatable }
+  attributes #1 = { nounwind }
+  
+  !llvm.module.flags = !{!0}
+  !llvm.dbg.cu = !{!1}
+  
+  !0 = !{i32 2, !"Debug Info Version", i32 3}
+  !1 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !2, producer: "beards", isOptimized: true, runtimeVersion: 4, emissionKind: FullDebug)
+  !2 = !DIFile(filename: "bees.cpp", directory: "")
+  !3 = distinct !DISubprogram(name: "nope", scope: !1, file: !2, line: 1, spFlags: DISPFlagDefinition, unit: !1, retainedNodes: !8)
+  !33 = distinct !DISubprogram(name: "alsonope", scope: !1, file: !2, line: 1, spFlags: DISPFlagDefinition, unit: !1, retainedNodes: !8)
+  !4 = !DILocalVariable(name: "bees", scope: !3, type: !5)
+  !5 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !6, size: 64)
+  !6 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !7 = !DILocation(line: 0, scope: !3)
+  !8 = !{!4}
+
+
+  ; CHECK: ![[METAVAR:[0-9]+]] = !DILocalVariable(name: "bees",
+
+...
+---
+name:            t
+# CHECK-LABEL: name: t
+tracksRegLiveness: true
+liveins:         
+  - { reg: '$edi', virtual-reg: '%2' }
+frameInfo:       
+  hasCalls:        true
+body:             |
+  bb.0.entry:
+    successors: %bb.1(0x40000000), %bb.2(0x40000000)
+    liveins: $edi
+  
+    ; Capture vreg num for subreg move for later checks; test that the COPY
+    ; of that vreg is optimized out.
+    ; CHECK-LABEL: bb.0.entry:
+    ; CHECK:       %[[BASEVREG:[0-9]+]]:gr64 = SUBREG_TO_REG
+    ; CHECK-NOT:   COPY %[[BASEVREG]]:gr64
+
+    %2:gr32 = COPY $edi
+    %3:gr32 = MOV32rr %2
+    %0:gr64 = SUBREG_TO_REG 0, killed %3, %subreg.sub_32bit
+    %4:gr64_nosp = SHL64ri %0, 9, implicit-def dead $eflags
+    %1:gr64 = LEA64r %4, 4, %4, 0, $noreg
+    %5:gr32 = MOV32r0 implicit-def dead $eflags
+    %6:gr8 = COPY %5.sub_8bit
+    %100:gr64 = COPY %0:gr64
+    TEST8rr %6, %6, implicit-def $eflags
+    JCC_1 %bb.2, 5, implicit $eflags
+    JMP_1 %bb.1
+  
+  bb.1.bb1:
+    successors: 
+  
+    ; Check for CSE happening and DBG_VALUE updating.
+    ; CHECK-LABEL: bb.1.bb1:
+    ; CHECK-NOT:   SHL64ri
+    ; CHECK-NOT:   LEA64r
+    ; CHECK:       DBG_VALUE %[[BASEVREG]], $noreg, ![[METAVAR]],
+    ; CHECK-NEXT:  ADJCALLSTACKDOWN64
+
+    %7:gr64_nosp = SHL64ri %100, 9, implicit-def dead $eflags
+    %8:gr64 = LEA64r %7, 4, %7, 0, $noreg
+    DBG_VALUE %100, $noreg, !4, !DIExpression(), debug-location !7
+    ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+    $rdi = COPY %8
+    CALL64pcrel32 @bar, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp
+    ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+  
+  bb.2.bb2:
+    ; As the COPY to %100 dies, the DBG_VALUE below should be updated too.
+    ; CHECK-LABEL: bb.2.bb2:
+    ; CHECK:       ADJCALLSTACKDOWN64
+    ; CHECK-NEXT:  $rdi = COPY
+    ; CHECK-NEXT:  DBG_VALUE %[[BASEVREG]], $noreg, ![[METAVAR]],
+    ; CHECK-NEXT:  CALL64pcrel32
+
+    ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+    $rdi = COPY %1
+    DBG_VALUE %100, $noreg, !4, !DIExpression(), debug-location !7
+    CALL64pcrel32 @baz, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp
+    ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+
+...
+---
+name:            u
+# CHECK-LABEL: name: u
+tracksRegLiveness: true
+liveins:         
+  - { reg: '$edi', virtual-reg: '%2' }
+frameInfo:       
+  hasCalls:        true
+body:             |
+  bb.0.entry:
+    successors: %bb.1(0x40000000), %bb.2(0x40000000)
+    liveins: $edi
+  
+    ; In this function, the COPY to %100 should not be optimized out, and as a
+    ; result the DBG_VALUEs should not be rewritten.
+    ; CHECK-LABEL: bb.0.entry:
+    ; CHECK:       %[[BASEVREG:[0-9]+]]:gr64 = SUBREG_TO_REG
+    ; CHECK:       %[[COPIEDVREG:[0-9]+]]:gr64 = COPY %[[BASEVREG]]
+
+    %2:gr32 = COPY $edi
+    %3:gr32 = MOV32rr %2
+    %0:gr64 = SUBREG_TO_REG 0, killed %3, %subreg.sub_32bit
+    %4:gr64_nosp = SHL64ri %0, 9, implicit-def dead $eflags
+    %1:gr64 = LEA64r %4, 4, %4, 0, $noreg
+    %5:gr32 = MOV32r0 implicit-def dead $eflags
+    %6:gr8 = COPY %5.sub_8bit
+    %100:gr64 = COPY %0:gr64
+    TEST8rr %6, %6, implicit-def $eflags
+    JCC_1 %bb.2, 5, implicit $eflags
+    JMP_1 %bb.1
+  
+  bb.1.bb1:
+    successors: 
+  
+    ; CSE should happen, DBG_VALUE updating should not.
+    ; CHECK-LABEL: bb.1.bb1:
+    ; CHECK-NOT:   SHL64ri
+    ; CHECK-NOT:   LEA64r
+    ; CHECK:       DBG_VALUE %[[COPIEDVREG]], $noreg, ![[METAVAR]],
+    ; CHECK-NEXT:  ADJCALLSTACKDOWN64
+
+
+
+    %7:gr64_nosp = SHL64ri %100, 9, implicit-def dead $eflags
+    %8:gr64 = LEA64r %7, 4, %7, 0, $noreg
+    DBG_VALUE %100, $noreg, !4, !DIExpression(), debug-location !7
+    ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+    $rdi = COPY %8
+    CALL64pcrel32 @bar, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp
+    ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+  
+  bb.2.bb2:
+
+    ; Test that the copy-read of %100 below is preserved, and the DBG_VALUE
+    ; operand is too.
+    ; CHECK-LABEL: bb.2.bb2:
+    ; CHECK:       ADJCALLSTACKDOWN64
+    ; CHECK-NEXT:  $rdi = COPY %[[COPIEDVREG]]
+    ; CHECK-NEXT:  DBG_VALUE %[[COPIEDVREG]], $noreg, ![[METAVAR]],
+    ; CHECK-NEXT:  CALL64pcrel32
+
+    ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+    $rdi = COPY %100
+    DBG_VALUE %100, $noreg, !4, !DIExpression(), debug-location !7
+    CALL64pcrel32 @baz, csr_64, implicit $rsp, implicit $ssp, implicit $rdi, implicit-def $rsp, implicit-def $ssp
+    ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
+
+...