[X86] Avoid codegen changes when DBG_VALUE appears between lowered selects

[jmorse]

Summary

X86TargetLowering::EmitLoweredSelect currently detects sequences of CMOV pseudo instructions without accounting for debug intrinsics. This leads to different codegen with and without option -g, if a DBG_VALUE instruction lands in the middle of several lowered selects, as demonstrated if you run the added test function below with trunk (additional block branching occurs).

Work around this by skipping over debug instructions when looking for CMOV sequences, and sinking those debug insts into the EmitLoweredSelect sunk block. This might slightly shift where variables appear in the instruction sequence, but won't re-order assignments.

[andreadb]

lib/Target/X86/X86ISelLowering.cpp

28780–28782 ↗

(On Diff #188356)

I think you can use function skipDebugInstructionsForward() to update that iterator. It would also make this code a bit more readable.

28821–28829 ↗

(On Diff #188356)

Here you iterate from LastCMOV to MI to search for any debug instructions to sink.
Why don't you collect those directly from the loop starting at line 28778.
Basically - if I understand this code correctly - you are scanning the code sequence twice.

[andreadb]

lib/Target/X86/X86ISelLowering.cpp

28821–28829 ↗

(On Diff #188356)

On a second thought. It may not be easy to do what I suggested in that comment. So, I am okay if you iterate instructions again.
You can probably move this loop before the splice at line 28817. The advantage is that you can perform a potentially more readable forward iteration. It would allow you to can replace calls to insert with calls to SinkMBB->push_back(CurInst).

[jmorse]

Incorporate feedback: use skipDebugInstructionsForward for better readability, iterate forwards when sinking DBG_VALUEs.

(We still have to juggle iterators when sinking the DBG_VALUEs as I believe the current one is invalidated when you call removeFromParent()).

[jmorse]

Incorporate recommendations on using auto from Andrea, delivered offline,

[andreadb]

LGTM

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

X86TargetLowering::EmitLoweredSelect(MachineInstr &MI,
   //
   // Case 2:
   // CMOV ((CMOV F, T, cc1), T, cc2) is checked here and handled by a separate
   // function - EmitLoweredCascadedSelect.
 
   X86::CondCode CC = X86::CondCode(MI.getOperand(3).getImm());
   X86::CondCode OppCC = X86::GetOppositeBranchCondition(CC);
   MachineInstr *LastCMOV = &MI;
-  MachineBasicBlock::iterator NextMIIt =
-      std::next(MachineBasicBlock::iterator(MI));
+  MachineBasicBlock::iterator NextMIIt = MachineBasicBlock::iterator(MI);
 
   // Check for case 1, where there are multiple CMOVs with the same condition
   // first.  Of the two cases of multiple CMOV lowerings, case 1 reduces the
   // number of jumps the most.
 
   if (isCMOVPseudo(MI)) {
-    // See if we have a string of CMOVS with the same condition.
+    // See if we have a string of CMOVS with the same condition. Skip over
+    // intervening debug insts.
     while (NextMIIt != ThisMBB->end() && isCMOVPseudo(*NextMIIt) &&
            (NextMIIt->getOperand(3).getImm() == CC ||
             NextMIIt->getOperand(3).getImm() == OppCC)) {
       LastCMOV = &*NextMIIt;
       ++NextMIIt;
+      NextMIIt = skipDebugInstructionsForward(NextMIIt, ThisMBB->end());
     }
   }
 
   // This checks for case 2, but only do this if we didn't already find
   // case 1, as indicated by LastCMOV == MI.
   if (LastCMOV == &MI && NextMIIt != ThisMBB->end() &&
       NextMIIt->getOpcode() == MI.getOpcode() &&
       NextMIIt->getOperand(2).getReg() == MI.getOperand(2).getReg() &&
X86TargetLowering::EmitLoweredSelect(MachineInstr &MI,
   // live into the sink and copy blocks.
   const TargetRegisterInfo *TRI = Subtarget.getRegisterInfo();
   if (!LastCMOV->killsRegister(X86::EFLAGS) &&
       !checkAndUpdateEFLAGSKill(LastCMOV, ThisMBB, TRI)) {
     FalseMBB->addLiveIn(X86::EFLAGS);
     SinkMBB->addLiveIn(X86::EFLAGS);
   }
 
+  // Transfer any debug instructions inside the CMOV sequence to the sunk block.
+  auto DbgEnd = MachineBasicBlock::iterator(LastCMOV);
+  auto DbgIt = MachineBasicBlock::iterator(MI);
+  while (DbgIt != DbgEnd) {
+    auto Next = std::next(DbgIt);
+    if (DbgIt->isDebugInstr())
+      SinkMBB->push_back(DbgIt->removeFromParent());
+    DbgIt = Next;
+  }
+
   // Transfer the remainder of ThisMBB and its successor edges to SinkMBB.
-  SinkMBB->splice(SinkMBB->begin(), ThisMBB,
+  SinkMBB->splice(SinkMBB->end(), ThisMBB,
                   std::next(MachineBasicBlock::iterator(LastCMOV)),
                   ThisMBB->end());
   SinkMBB->transferSuccessorsAndUpdatePHIs(ThisMBB);
 
   // Fallthrough block for ThisMBB.
   ThisMBB->addSuccessor(FalseMBB);
   // The true block target of the first (or only) branch is always a SinkMBB.
   ThisMBB->addSuccessor(SinkMBB);

llvm/trunk/test/CodeGen/X86/pseudo_cmov_lower2.ll

entry:
   %d2 = select i1 %c1, double %d0, double %d1, !prof !0
   %d3 = select i1 %c2, double %d2, double %p2, !prof !0
   %d4 = select i1 %c3, double %d3, double %p3, !prof !0
   %d5 = fsub double %d2, %d3
   %d6 = fadd double %d5, %d4
   ret double %d6
 }
 
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+; Like the test for @foo1, but check that the inserted dbg.value does not
+; affect codegen. The CHECK items below should always be identical to @foo1,
+; minus the DEBUG_VALUE line and changes in labels..
+define double @foo1_g(float %p1, double %p2, double %p3) nounwind !dbg !4 {
+; CHECK-LABEL: foo1_g:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    xorps %xmm3, %xmm3
+; CHECK-NEXT:    ucomiss %xmm3, %xmm0
+; CHECK-NEXT:    movsd {{.*#+}} xmm0 = mem[0],zero
+; CHECK-NEXT:    jae .LBB6_1
+; CHECK-NEXT:  # %bb.2: # %entry
+; CHECK-NEXT:    addsd %xmm2, %xmm0
+; CHECK-NEXT:    jmp .LBB6_3
+; CHECK-NEXT:  .LBB6_1:
+; CHECK-NEXT:    addsd %xmm0, %xmm1
+; CHECK-NEXT:    movapd %xmm1, %xmm0
+; CHECK-NEXT:    movapd %xmm1, %xmm2
+; CHECK-NEXT:  .LBB6_3: # %entry
+; CHECK-NEXT:    #DEBUG_VALUE: foobar:xyzzy <- undef
+; CHECK-NEXT:    subsd %xmm1, %xmm0
+; CHECK-NEXT:    addsd %xmm2, %xmm0
+; CHECK-NEXT:    retq
+entry:
+  %c1 = fcmp oge float %p1, 0.000000e+00
+  %d0 = fadd double %p2, 1.25e0
+  %d1 = fadd double %p3, 1.25e0
+  %d2 = select i1 %c1, double %d0, double %d1
+  call void @llvm.dbg.value(metadata float undef, metadata !5, metadata !DIExpression()), !dbg !6
+  %d3 = select i1 %c1, double %d2, double %p2
+  %d4 = select i1 %c1, double %d3, double %p3
+  %d5 = fsub double %d2, %d3
+  %d6 = fadd double %d5, %d4
+  ret double %d6
+}
+
+!llvm.module.flags = !{!1}
+!llvm.dbg.cu = !{!2}
+
 !0 = !{!"branch_weights", i32 1, i32 2000}
+!1 = !{i32 2, !"Debug Info Version", i32 3}
+!2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, nameTableKind: None)
+!3 = !DIFile(filename: "test.c", directory: ".")
+!4 = distinct !DISubprogram(name: "foobar", scope: !2, file: !3, line: 1, type: !9, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !2, retainedNodes: !8)
+!5 = !DILocalVariable(name: "xyzzy", scope: !4, file: !3, line: 2, type: !7)
+!6 = !DILocation(line: 1, column: 1, scope: !4)
+!7 = !DIBasicType(name: "float", size: 32, encoding: DW_ATE_float)
+!8 = !{!5}
+!9 = !DISubroutineType(types: !10)
+!10 = !{!7}