Machine sink could potentially sink instructions into a block where the physical

registers it defines then interfere with an existing preg live range.

For instance, if we had something like these machine instructions:

BB#0
  ... = imul ... EFLAGS<imp-def,dead>
  test ..., EFLAGS<imp-def>
  jcc BB#2 EFLAGS<imp-use>

BB#1
  ... ; fallthrough to BB#2

BB#2
  ... ; No code that defines EFLAGS
  jcc ... EFLAGS<imp-use>

Machine sink will come along, see that imul implicitly defines EFLAGS, but
because it's "dead", it assumes that it can move imul into BB#2. But when it
does, imul's "dead" imp-def of EFLAGS is raised from the dead (a zombie) and
messes up the condition code for the jump (and pretty much anything else which
relies upon it being correct).

The solution is to know which pregs are live going into a basic block. However,
that information isn't calculated at this point. Nor does the LiveVariables pass
take into account non-allocatable physical registers. In lieu of this, we do a
*very* conservative pass through the basic block to determine if a preg is live
coming out of it.


git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@105387 91177308-0d34-0410-b5e6-96231b3b80d8

3 files changed, 85 insertions, 20 deletions

diff --git a/lib/CodeGen/MachineSink.cpp b/lib/CodeGen/MachineSink.cpp
index 72676faa89..7b4bc15d8c 100644
--- a/lib/CodeGen/MachineSink.cpp
+++ b/lib/CodeGen/MachineSink.cpp
@@ -25,6 +25,7 @@
 #include "llvm/Target/TargetRegisterInfo.h"
 #include "llvm/Target/TargetInstrInfo.h"
 #include "llvm/Target/TargetMachine.h"
+#include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
@@ -61,6 +62,7 @@ namespace {
     bool ProcessBlock(MachineBasicBlock &MBB);
     bool SinkInstruction(MachineInstr *MI, bool &SawStore);
     bool AllUsesDominatedByBlock(unsigned Reg, MachineBasicBlock *MBB) const;
+    bool LiveOutOfBasicBlock(const MachineInstr *MI, unsigned Reg) const;
   };
 } // end anonymous namespace
   
@@ -166,6 +168,44 @@ bool MachineSinking::ProcessBlock(MachineBasicBlock &MBB) {
   return MadeChange;
 }
 
+/// LiveOutOfBasicBlock - Determine if the physical register, defined and dead
+/// in MI, is live on exit from the basic block.
+bool MachineSinking::LiveOutOfBasicBlock(const MachineInstr *MI,
+                                         unsigned Reg) const {
+  assert(TargetRegisterInfo::isPhysicalRegister(Reg) &&
+         "Only want to determine if a physical register is live out of a BB!");
+
+  const MachineBasicBlock *MBB = MI->getParent();
+  SmallSet<unsigned, 8> KilledRegs;
+  MachineBasicBlock::const_iterator I = MBB->end();
+  MachineBasicBlock::const_iterator E = MBB->begin();
+  assert(I != E && "How can there be an empty block at this point?!");
+
+  // Loop through the instructions bottom-up. If we see a kill of the preg
+  // first, then it's not live out of the BB. If we see a use or def first, then
+  // we assume that it is live out of the BB.
+  do {
+    const MachineInstr &CurMI = *--I;
+
+    for (unsigned i = 0, e = CurMI.getNumOperands(); i != e; ++i) {
+      const MachineOperand &MO = CurMI.getOperand(i);
+      if (!MO.isReg()) continue;  // Ignore non-register operands.
+    
+      unsigned MOReg = MO.getReg();
+      if (MOReg == 0) continue;
+    
+      if (MOReg == Reg) {
+        if (MO.isKill())
+          return false;
+        if (MO.isUse() || MO.isDef())
+          return true;
+      }
+    }
+  } while (I != E);
+
+  return false;
+}
+
 /// SinkInstruction - Determine whether it is safe to sink the specified machine
 /// instruction out of its current block into a successor.
 bool MachineSinking::SinkInstruction(MachineInstr *MI, bool &SawStore) {
@@ -188,6 +228,7 @@ bool MachineSinking::SinkInstruction(MachineInstr *MI, bool &SawStore) {
   // SuccToSinkTo - This is the successor to sink this instruction to, once we
   // decide.
   MachineBasicBlock *SuccToSinkTo = 0;
+  SmallVector<unsigned, 4> PhysRegs;
   
   for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
     const MachineOperand &MO = MI->getOperand(i);
@@ -216,9 +257,12 @@ bool MachineSinking::SinkInstruction(MachineInstr *MI, bool &SawStore) {
           if (AllocatableSet.test(AliasReg))
             return false;
         }
-      } else if (!MO.isDead()) {
-        // A def that isn't dead. We can't move it.
-        return false;
+      } else {
+        if (!MO.isDead())
+          // A def that isn't dead. We can't move it.
+          return false;
+        else
+          PhysRegs.push_back(Reg);
       }
     } else {
       // Virtual register uses are always safe to sink.
@@ -281,7 +325,15 @@ bool MachineSinking::SinkInstruction(MachineInstr *MI, bool &SawStore) {
   // happen with loops.
   if (MI->getParent() == SuccToSinkTo)
     return false;
-  
+
+  // If the instruction to move defines a dead physical register which is live
+  // when leaving the basic block, don't move it because it could turn into a
+  // "zombie" define of that preg. E.g., EFLAGS. (<rdar://problem/8030636>)
+  for (SmallVectorImpl<unsigned>::const_iterator
+         I = PhysRegs.begin(), E = PhysRegs.end(); I != E; ++I)
+    if (LiveOutOfBasicBlock(MI, *I))
+      return false;
+
   DEBUG(dbgs() << "Sink instr " << *MI << "\tinto block " << *SuccToSinkTo);
 
   // If the block has multiple predecessors, this would introduce computation on
diff --git a/test/CodeGen/X86/MachineSink-CritEdge.ll b/test/CodeGen/X86/MachineSink-CritEdge.ll
index 74a1049772..db06b329f9 100644
--- a/test/CodeGen/X86/MachineSink-CritEdge.ll
+++ b/test/CodeGen/X86/MachineSink-CritEdge.ll
@@ -1,4 +1,10 @@
 ; RUN: llc < %s | FileCheck %s
+; XFAIL: *
+;
+; See <rdar://problem/8030636>. This test isn't valid after we made machine
+; sinking more conservative about sinking instructions that define a preg into a
+; block when we don't know if the preg is killed within the current block.
+
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
 target triple = "x86_64-apple-darwin10.0.0"
 
diff --git a/test/CodeGen/X86/sink-hoist.ll b/test/CodeGen/X86/sink-hoist.ll
index 031c01e9af..acba5288c0 100644
--- a/test/CodeGen/X86/sink-hoist.ll
+++ b/test/CodeGen/X86/sink-hoist.ll
@@ -44,26 +44,33 @@ return:
 
 ; Sink instructions with dead EFLAGS defs.
 
-; CHECK: zzz:
-; CHECK:      je
-; CHECK-NEXT: orb
-
-define zeroext i8 @zzz(i8 zeroext %a, i8 zeroext %b) nounwind readnone {
-entry:
-  %tmp = zext i8 %a to i32                        ; <i32> [#uses=1]
-  %tmp2 = icmp eq i8 %a, 0                    ; <i1> [#uses=1]
-  %tmp3 = or i8 %b, -128                          ; <i8> [#uses=1]
-  %tmp4 = and i8 %b, 127                          ; <i8> [#uses=1]
-  %b_addr.0 = select i1 %tmp2, i8 %tmp4, i8 %tmp3 ; <i8> [#uses=1]
-  ret i8 %b_addr.0
-}
+; FIXME: Unfail the zzz test if we can correctly mark pregs with the kill flag.
+; 
+; See <rdar://problem/8030636>. This test isn't valid after we made machine
+; sinking more conservative about sinking instructions that define a preg into a
+; block when we don't know if the preg is killed within the current block.
+
+
+; FIXMEHECK: zzz:
+; FIXMEHECK:      je
+; FIXMEHECK-NEXT: orb
+
+; define zeroext i8 @zzz(i8 zeroext %a, i8 zeroext %b) nounwind readnone {
+; entry:
+;   %tmp = zext i8 %a to i32                        ; <i32> [#uses=1]
+;   %tmp2 = icmp eq i8 %a, 0                    ; <i1> [#uses=1]
+;   %tmp3 = or i8 %b, -128                          ; <i8> [#uses=1]
+;   %tmp4 = and i8 %b, 127                          ; <i8> [#uses=1]
+;   %b_addr.0 = select i1 %tmp2, i8 %tmp4, i8 %tmp3 ; <i8> [#uses=1]
+;   ret i8 %b_addr.0
+; }
 
 ; Codegen should hoist and CSE these constants.
 
 ; CHECK: vv:
-; CHECK: LCPI3_0(%rip), %xmm0
-; CHECK: LCPI3_1(%rip), %xmm1
-; CHECK: LCPI3_2(%rip), %xmm2
+; CHECK: LCPI2_0(%rip), %xmm0
+; CHECK: LCPI2_1(%rip), %xmm1
+; CHECK: LCPI2_2(%rip), %xmm2
 ; CHECK: align
 ; CHECK-NOT: LCPI
 ; CHECK: ret