From 869d60d39d579b2051a8e34f460de72f071c2172 Mon Sep 17 00:00:00 2001 From: Bill Wendling Date: Thu, 3 Jun 2010 07:54:20 +0000 Subject: [PATCH] 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 test ..., EFLAGS jcc BB#2 EFLAGS BB#1 ... ; fallthrough to BB#2 BB#2 ... ; No code that defines EFLAGS jcc ... EFLAGS 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 --- lib/CodeGen/MachineSink.cpp | 60 ++++++++++++++++++++++-- test/CodeGen/X86/MachineSink-CritEdge.ll | 6 +++ test/CodeGen/X86/sink-hoist.ll | 37 +++++++++------ 3 files changed, 84 insertions(+), 19 deletions(-) diff --git a/lib/CodeGen/MachineSink.cpp b/lib/CodeGen/MachineSink.cpp index 72676faa899..7b4bc15d8c0 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 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 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. () + for (SmallVectorImpl::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 74a1049772a..db06b329f9b 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 . 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 031c01e9af7..acba5288c0d 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 +; FIXME: Unfail the zzz test if we can correctly mark pregs with the kill flag. +; +; See . 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. -define zeroext i8 @zzz(i8 zeroext %a, i8 zeroext %b) nounwind readnone { -entry: - %tmp = zext i8 %a to i32 ; [#uses=1] - %tmp2 = icmp eq i8 %a, 0 ; [#uses=1] - %tmp3 = or i8 %b, -128 ; [#uses=1] - %tmp4 = and i8 %b, 127 ; [#uses=1] - %b_addr.0 = select i1 %tmp2, i8 %tmp4, i8 %tmp3 ; [#uses=1] - ret i8 %b_addr.0 -} + +; 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 ; [#uses=1] +; %tmp2 = icmp eq i8 %a, 0 ; [#uses=1] +; %tmp3 = or i8 %b, -128 ; [#uses=1] +; %tmp4 = and i8 %b, 127 ; [#uses=1] +; %b_addr.0 = select i1 %tmp2, i8 %tmp4, i8 %tmp3 ; [#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