[DebugInfo][InstrRef] Instrument x86 CMOV conversion to preserve variable values

[jmorse]

Summary

X86's CMOV conversion transforms CMOV instructions into control flow between blocks, meaning the value is computed by a PHI rather than a "real" machine instruction. In instruction-referencing mode, we need to transfer the instruction label between the old CMOV and the new PHI instruction to mark where the variable value is computed.

There's an extra complication in that memory operands can be unfolded from the CMOV and sunk into the new blocks -- the test checks both scenarios where the instruction number has to hop between instructions.

This omission exposed by Dexter testing :O !

[Orlando]

Nice! LGTM with nit / question inline.

llvm/test/DebugInfo/MIR/InstrRef/x86-cmov-converter.mir

9

Do these CHECK lines cover both functions / part of the test? IMO it's not entirely clear. Maybe it's worth adding another CHECK-LABEL to improve clarity?

[Orlando]

The verifier issue should be fixed in D153950. Slightly surprised that this test is the first to find the issue... grepping for x86-cmov-conversion in llvm/test only shows this new test.

llvm/lib/Target/X86/X86CmovConversion.cpp

for (MachineBasicBlock::iterator MIIt = MIItBegin; MIIt != MIItEnd;) {
     //     %C = MOV (mem)
     //   SinkMBB:
     //     %A = PHI [ %C, FalseMBB ], [ %B, MBB]
 
     // Get a fresh register to use as the destination of the MOV.
     const TargetRegisterClass *RC = MRI->getRegClass(MI.getOperand(0).getReg());
     Register TmpReg = MRI->createVirtualRegister(RC);
 
+    // Retain debug instr number when unfolded.
+    unsigned OldDebugInstrNum = MI.peekDebugInstrNum();
     SmallVector<MachineInstr *, 4> NewMIs;
     bool Unfolded = TII->unfoldMemoryOperand(*MBB->getParent(), MI, TmpReg,
                                              /*UnfoldLoad*/ true,
                                              /*UnfoldStore*/ false, NewMIs);
     (void)Unfolded;
     assert(Unfolded && "Should never fail to unfold a loading cmov!");
 
     // Move the new CMOV to just before the old one and reset any impacted
     // iterator.
     auto *NewCMOV = NewMIs.pop_back_val();
     assert(X86::getCondFromCMov(*NewCMOV) == OppCC &&
            "Last new instruction isn't the expected CMOV!");
     LLVM_DEBUG(dbgs() << "\tRewritten cmov: "; NewCMOV->dump());
     MBB->insert(MachineBasicBlock::iterator(MI), NewCMOV);
     if (&*MIItBegin == &MI)
       MIItBegin = MachineBasicBlock::iterator(NewCMOV);
 
+    if (OldDebugInstrNum)
+      NewCMOV->setDebugInstrNum(OldDebugInstrNum);
+
     // Sink whatever instructions were needed to produce the unfolded operand
     // into the false block.
     for (auto *NewMI : NewMIs) {
       LLVM_DEBUG(dbgs() << "\tRewritten load instr: "; NewMI->dump());
       FalseMBB->insert(FalseInsertionPoint, NewMI);
       // Re-map any operands that are from other cmovs to the inputs for this block.
       for (auto &MOp : NewMI->uses()) {
         if (!MOp.isReg())
MIB = BuildMI(*SinkMBB, SinkInsertionPoint, DL, TII->get(X86::PHI), DestReg)
               .addReg(Op1Reg)
               .addMBB(FalseMBB)
               .addReg(Op2Reg)
               .addMBB(MBB);
     (void)MIB;
     LLVM_DEBUG(dbgs() << "\tFrom: "; MIIt->dump());
     LLVM_DEBUG(dbgs() << "\tTo: "; MIB->dump());
 
+    // debug-info: we can just copy the instr-ref number from one instruction
+    // to the other, seeing how it's a one-for-one substitution.
+    if (unsigned InstrNum = MIIt->peekDebugInstrNum())
+      MIB->setDebugInstrNum(InstrNum);
+
     // Add this PHI to the rewrite table.
     RegRewriteTable[DestReg] = std::make_pair(Op1Reg, Op2Reg);
   }
 
   // Now remove the CMOV(s).
   MBB->erase(MIItBegin, MIItEnd);
 
   // Add new basic blocks to MachineLoopInfo.

llvm/test/DebugInfo/MIR/InstrRef/x86-cmov-converter.mir

+# RUN: llc %s -o - --run-pass=x86-cmov-conversion -mtriple=x86_64-- | FileCheck %s
+# REQUIRES: x86-registered-target
+#
+# Check that when we create new blocks and PHIs to represent CMOVs, that the
+# debug-instr-number is replaced on the new instruction, to preserve variable
+# locations. Check that this still works when unfolding memory operands, which
+# involves more decomposition of instructions.
+#
+# CHECK-LABEL: name: CmovInHotPath
+# CHECK-LABEL: bb.3.for.body:
+# CHECK: CMOV32rr {{.*}}, debug-instr-number 1
+#
+# CHECK-LABEL: name: test_cmov_memoperand_in_group_reuse_for_addr2
+# CHECK-LABEL: bb.2.entry:
+# CHECK-NEXT:  PHI {{.*}} debug-instr-number 1,
+# CHECK-NEXT:  PHI {{.*}} debug-instr-number 2,
+--- |
+  ; ModuleID = 'x86-cmov-converter.ll'
+  source_filename = "x86-cmov-converter.ll"
+  target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+
+  ; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
+  declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+
+  define void @CmovInHotPath(i32 %n, i32 %a, i32 %b, ptr nocapture %c, ptr nocapture readnone %d) !dbg !7 {
+  entry:
+    %cmp14 = icmp sgt i32 %n, 0, !dbg !12
+    br i1 %cmp14, label %for.body.preheader, label %for.cond.cleanup
+
+  for.body.preheader:                               ; preds = %entry
+    %wide.trip.count = zext i32 %n to i64, !dbg !13
+    br label %for.body
+
+  for.cond.cleanup:                                 ; preds = %for.body, %entry
+    ret void
+
+  for.body:                                         ; preds = %for.body, %for.body.preheader
+    %lsr.iv1 = phi ptr [ %uglygep, %for.body ], [ %c, %for.body.preheader ]
+    %lsr.iv = phi i64 [ %lsr.iv.next, %for.body ], [ %wide.trip.count, %for.body.preheader ]
+    %0 = load i32, ptr %lsr.iv1, align 4, !dbg !13
+    %add = add nsw i32 %0, 1, !dbg !13
+    %mul = mul nsw i32 %0, %a, !dbg !13
+    %cmp3 = icmp sgt i32 %mul, %b, !dbg !13
+    %. = select i1 %cmp3, i32 10, i32 %add, !dbg !13
+    call void @llvm.dbg.value(metadata i32 %., metadata !14, metadata !DIExpression()), !dbg !13
+    %mul7 = mul nsw i32 %., %add, !dbg !13
+    store i32 %mul7, ptr %lsr.iv1, align 4, !dbg !13
+    %lsr.iv.next = add nsw i64 %lsr.iv, -1, !dbg !13
+    %uglygep = getelementptr i8, ptr %lsr.iv1, i64 4, !dbg !13
+    %exitcond = icmp eq i64 %lsr.iv.next, 0, !dbg !13
+    br i1 %exitcond, label %for.cond.cleanup, label %for.body, !dbg !13
+  }
+
+  define i32 @test_cmov_memoperand_in_group_reuse_for_addr2(i32 %a, i32 %b, ptr %x, ptr %y) !dbg !15 {
+  entry:
+    %cond = icmp ugt i32 %a, %b, !dbg !16
+    %load1 = load ptr, ptr %y, align 8, !dbg !16
+    %p = select i1 %cond, ptr %x, ptr %load1, !dbg !16
+    call void @llvm.dbg.value(metadata ptr %p, metadata !17, metadata !DIExpression()), !dbg !16
+    %load2 = load i32, ptr %p, align 4, !dbg !16
+    %z = select i1 %cond, i32 %a, i32 %load2, !dbg !16
+    call void @llvm.dbg.value(metadata i32 %z, metadata !18, metadata !DIExpression()), !dbg !16
+    ret i32 %z, !dbg !16
+  }
+
+  attributes #0 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }
+
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!3, !4, !5}
+  !llvm.ident = !{!6}
+
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+  !1 = !DIFile(filename: "test.c", directory: "/tmp/out.c")
+  !2 = !{}
+  !3 = !{i32 7, !"Dwarf Version", i32 4}
+  !4 = !{i32 2, !"Debug Info Version", i32 3}
+  !5 = !{i32 1, !"wchar_size", i32 4}
+  !6 = !{!""}
+  !7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 3, type: !8, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+  !8 = !DISubroutineType(types: !9)
+  !9 = !{!10, !11, !11}
+  !10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !11 = !DIBasicType(name: "long int", size: 64, encoding: DW_ATE_signed)
+  !12 = !DILocation(line: 16, column: 3, scope: !7)
+  !13 = !DILocation(line: 15, column: 3, scope: !7)
+  !14 = !DILocalVariable(name: "bar", arg: 1, scope: !7, file: !1, line: 3, type: !11)
+  !15 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 3, type: !8, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+  !16 = !DILocation(line: 16, column: 3, scope: !15)
+  !17 = !DILocalVariable(name: "bar", arg: 1, scope: !15, file: !1, line: 3, type: !11)
+  !18 = !DILocalVariable(name: "baz", arg: 2, scope: !15, file: !1, line: 3, type: !11)
+
+...
+---
+name:            CmovInHotPath
+tracksRegLiveness: true
+debugInstrRef:   true
+registers:
+  - { id: 0, class: gr64, preferred-register: '' }
+  - { id: 1, class: gr64, preferred-register: '' }
+  - { id: 2, class: gr64, preferred-register: '' }
+  - { id: 3, class: gr64, preferred-register: '' }
+  - { id: 4, class: gr64, preferred-register: '' }
+  - { id: 5, class: gr32, preferred-register: '' }
+  - { id: 6, class: gr32, preferred-register: '' }
+  - { id: 7, class: gr32, preferred-register: '' }
+liveins:
+  - { reg: '$edi', virtual-reg: '%5' }
+  - { reg: '$esi', virtual-reg: '%6' }
+  - { reg: '$edx', virtual-reg: '%7' }
+  - { reg: '$rcx', virtual-reg: '%8' }
+machineFunctionInfo: {}
+body:             |
+  bb.0.entry:
+    successors: %bb.1(0x50000000), %bb.2(0x30000000)
+    liveins: $edi, $esi, $edx, $rcx
+
+    %8:gr64 = COPY $rcx
+    %7:gr32 = COPY $edx
+    %6:gr32 = COPY $esi
+    %5:gr32 = COPY $edi
+    TEST32rr %5, %5, implicit-def $eflags, debug-location !12
+    JCC_1 %bb.2, 14, implicit $eflags
+    JMP_1 %bb.1
+
+  bb.1.for.body.preheader:
+    successors: %bb.3(0x80000000)
+
+    %10:gr32 = MOV32rr %5, debug-location !13
+    %0:gr64 = SUBREG_TO_REG 0, killed %10, %subreg.sub_32bit, debug-location !13
+    JMP_1 %bb.3
+
+  bb.2.for.cond.cleanup:
+    RET 0
+
+  bb.3.for.body:
+    successors: %bb.2(0x04000000), %bb.3(0x7c000000)
+
+    %1:gr64 = PHI %8, %bb.1, %4, %bb.3
+    %2:gr64 = PHI %0, %bb.1, %3, %bb.3
+    %11:gr32 = MOV32rm %1, 1, $noreg, 0, $noreg, debug-location !13 :: (load (s32) from %ir.lsr.iv1)
+    %12:gr32 = nsw INC32r %11, implicit-def dead $eflags, debug-location !13
+    %13:gr32 = nsw IMUL32rr %11, %6, implicit-def dead $eflags, debug-location !13
+    %14:gr32 = SUB32rr %13, %7, implicit-def $eflags, debug-location !13
+    %15:gr32 = MOV32ri 10
+    %16:gr32 = CMOV32rr %12, killed %15, 15, implicit $eflags, debug-instr-number 1, debug-location !13
+    DBG_INSTR_REF !14, !DIExpression(DW_OP_LLVM_arg, 0), dbg-instr-ref(1, 0), debug-location !13
+    %17:gr32 = nsw IMUL32rr %16, %12, implicit-def dead $eflags, debug-location !13
+    MOV32mr %1, 1, $noreg, 0, $noreg, killed %17, debug-location !13 :: (store (s32) into %ir.lsr.iv1)
+    %4:gr64 = ADD64ri8 %1, 4, implicit-def dead $eflags, debug-location !13
+    %3:gr64 = DEC64r %2, implicit-def $eflags, debug-location !13
+    JCC_1 %bb.2, 4, implicit $eflags, debug-location !13
+    JMP_1 %bb.3, debug-location !13
+
+...
+---
+name:            test_cmov_memoperand_in_group_reuse_for_addr2
+alignment:       16
+tracksRegLiveness: true
+debugInstrRef:   true
+registers:
+  - { id: 0, class: gr32, preferred-register: '' }
+  - { id: 1, class: gr32, preferred-register: '' }
+  - { id: 2, class: gr64, preferred-register: '' }
+  - { id: 3, class: gr64, preferred-register: '' }
+  - { id: 4, class: gr32, preferred-register: '' }
+  - { id: 5, class: gr64, preferred-register: '' }
+  - { id: 6, class: gr32, preferred-register: '' }
+liveins:
+  - { reg: '$edi', virtual-reg: '%0' }
+  - { reg: '$esi', virtual-reg: '%1' }
+  - { reg: '$rdx', virtual-reg: '%2' }
+  - { reg: '$rcx', virtual-reg: '%3' }
+debugValueSubstitutions: []
+constants:       []
+machineFunctionInfo: {}
+body:             |
+  bb.0.entry:
+    liveins: $edi, $esi, $rdx, $rcx
+
+    %3:gr64 = COPY $rcx
+    %2:gr64 = COPY $rdx
+    %1:gr32 = COPY $esi
+    %0:gr32 = COPY $edi
+    %4:gr32 = SUB32rr %0, %1, implicit-def $eflags, debug-location !16
+    %5:gr64 = CMOV64rm %2, %3, 1, $noreg, 0, $noreg, 6, implicit $eflags, debug-instr-number 1, debug-location !16 :: (load (s64) from %ir.y)
+    DBG_INSTR_REF !17, !DIExpression(DW_OP_LLVM_arg, 0), dbg-instr-ref(1, 0), debug-location !16
+    %6:gr32 = CMOV32rm %0, killed %5, 1, $noreg, 0, $noreg, 6, implicit $eflags, debug-instr-number 2, debug-location !16 :: (load (s32) from %ir.p)
+    DBG_INSTR_REF !18, !DIExpression(DW_OP_LLVM_arg, 0), dbg-instr-ref(2, 0), debug-location !16
+    $eax = COPY %6, debug-location !16
+    RET 0, $eax, debug-location !16
+
+...