If-conversion incorrectly calculates liveness of redefined registers

[kparzysz]

Summary

The if-convert-diamond function in the if-conversion initialized Redefs with the live-ins from the first of the two predicated blocks. This may not reflect the liveness of the defined registers correctly, since a register defined in the first block may be live across the other predicated block. The testcase illustrates this situation.

[kparzysz]

Remove a leftover debug statement.

[MatzeB]

LGTM. That was a subtle issue, thanks.

Some nitpicks below:

lib/CodeGen/IfConversion.cpp

1435–1437

I don't understand the new comment (the old comment was pretty bad too though).

How about something in the line of:

// - BB2 live-in regs need implicit uses before being redefined by BB1 instructions.
// - BB1 live-out regs need implicit uses before being redefined by BB2 instructions;
//    We start with BB1 live-ins so we have the live-out regs after tracking the BB1 instructions.

test/CodeGen/Hexagon/ifcvt-impuse-livein.mir

2

Nice small and to the point test!

Would be nice if you could add one sentence about what is tested.
I would also recommend a # CHECK-LABEL: name: foo (just in case more tests get added later).

17

The %r1 livein looks unnecessary.

lib/CodeGen/IfConversion.cpp

 /// values defined in MI which are also live/used by MI.
 static void UpdatePredRedefs(MachineInstr &MI, LivePhysRegs &Redefs) {
   const TargetRegisterInfo *TRI = MI.getParent()->getParent()
     ->getSubtarget().getRegisterInfo();
 
   // Before stepping forward past MI, remember which regs were live
   // before MI. This is needed to set the Undef flag only when reg is
   // dead.
+dbgs() << __func__ << "  Redefs:" << Redefs << "  MI:" << MI;
   SparseSet<unsigned> LiveBeforeMI;
   LiveBeforeMI.setUniverse(TRI->getNumRegs());
   for (auto &Reg : Redefs)
     LiveBeforeMI.insert(Reg);
 
   SmallVector<std::pair<unsigned, const MachineOperand*>, 4> Clobbers;
   Redefs.stepForward(MI, Clobbers);
 
bool IfConverter::IfConvertDiamond(BBInfo &BBI, IfcvtKind Kind,
   if (DoSwap) {
     std::swap(BBI1, BBI2);
     std::swap(Cond1, Cond2);
   }
 
   // Remove the conditional branch from entry to the blocks.
   BBI.NonPredSize -= TII->RemoveBranch(*BBI.BB);
 
-  // Initialize liveins to the first BB. These are potentially redefined by
-  // predicated instructions.
+  // Initialize Redefs to be the live-outs from the split block. These are
+  // potentially redefined by predicated instructions.
   Redefs.init(TRI);
   Redefs.addLiveIns(*BBI1->BB);
+  Redefs.addLiveIns(*BBI2->BB);
 
   // Remove the duplicated instructions at the beginnings of both paths.
   // Skip dbg_value instructions
   MachineBasicBlock::iterator DI1 = BBI1->BB->getFirstNonDebugInstr();
   MachineBasicBlock::iterator DI2 = BBI2->BB->getFirstNonDebugInstr();
   BBI1->NonPredSize -= NumDups1;
   BBI2->NonPredSize -= NumDups1;
 

test/CodeGen/Hexagon/ifcvt-impuse-livein.mir

+# RUN: llc -march=hexagon -run-pass if-converter %s -o - | FileCheck %s
+
+--- |
+  define void @foo() {
+    ret void
+  }
+...
+
+---
+name: foo
+tracksRegLiveness: true
+allVRegsAllocated: true
+body: |
+  bb.0:
+    successors: %bb.1, %bb.2
+    liveins: %r0, %r1, %r2, %p1
+        J2_jumpf %p1, %bb.1, implicit-def %pc
+        J2_jump %bb.2, implicit-def %pc
+  bb.1:
+    successors: %bb.3
+    liveins: %r2
+        %r0 = A2_tfrsi 2
+        J2_jump %bb.3, implicit-def %pc
+  bb.2:
+    successors: %bb.3
+    liveins: %r0
+    ; CHECK: %r2 = C2_cmoveit %p1, 1, implicit %r2
+        %r2 = A2_tfrsi 1
+  bb.3:
+    liveins: %r0, %r2
+        %r0 = A2_add %r0, %r2
+        J2_jumpr %r31, implicit-def %pc
+...
+