[llvm-commits] [llvm] r106066 - in /llvm/trunk: lib/CodeGen/MachineSink.cpp lib/Target/X86/X86ISelLowering.cpp test/CodeGen/X86/MachineSink-CritEdge.ll

Daniel Dunbar daniel at zuster.org
Tue Jun 22 17:45:26 PDT 2010


Hi Bill,

It looks like bzip2 is being miscompiled with this commit in, I
reverted it in r106610.

Can you take a look?

 - Daniel

On Tue, Jun 15, 2010 at 4:46 PM, Bill Wendling <isanbard at gmail.com> wrote:
> Author: void
> Date: Tue Jun 15 18:46:31 2010
> New Revision: 106066
>
> URL: http://llvm.org/viewvc/llvm-project?rev=106066&view=rev
> Log:
> Create a more targeted fix for not sinking instructions into a range where it
> will conflict with another live range. The place which creates this scenerio is
> the code in X86 that lowers a select instruction by splitting the MBBs. This
> eliminates the need to check from the bottom up in an MBB for live pregs.
>
>
> Modified:
>    llvm/trunk/lib/CodeGen/MachineSink.cpp
>    llvm/trunk/lib/Target/X86/X86ISelLowering.cpp
>    llvm/trunk/test/CodeGen/X86/MachineSink-CritEdge.ll
>
> Modified: llvm/trunk/lib/CodeGen/MachineSink.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/MachineSink.cpp?rev=106066&r1=106065&r2=106066&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/MachineSink.cpp (original)
> +++ llvm/trunk/lib/CodeGen/MachineSink.cpp Tue Jun 15 18:46:31 2010
> @@ -25,7 +25,6 @@
>  #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"
> @@ -62,7 +61,6 @@
>     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
>
> @@ -168,44 +166,6 @@
>   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) {
> @@ -228,7 +188,6 @@
>   // 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);
> @@ -257,12 +216,9 @@
>           if (AllocatableSet.test(AliasReg))
>             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 if (!MO.isDead()) {
> +        // A def that isn't dead. We can't move it.
> +        return false;
>       }
>     } else {
>       // Virtual register uses are always safe to sink.
> @@ -329,10 +285,14 @@
>   // 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))
> +  for (unsigned I = 0, E = MI->getNumOperands(); I != E; ++I) {
> +    const MachineOperand &MO = MI->getOperand(I);
> +    if (!MO.isReg()) continue;
> +    unsigned Reg = MO.getReg();
> +    if (Reg == 0 || !TargetRegisterInfo::isPhysicalRegister(Reg)) continue;
> +    if (SuccToSinkTo->isLiveIn(Reg))
>       return false;
> +  }
>
>   DEBUG(dbgs() << "Sink instr " << *MI << "\tinto block " << *SuccToSinkTo);
>
>
> Modified: llvm/trunk/lib/Target/X86/X86ISelLowering.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.cpp?rev=106066&r1=106065&r2=106066&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/X86/X86ISelLowering.cpp (original)
> +++ llvm/trunk/lib/Target/X86/X86ISelLowering.cpp Tue Jun 15 18:46:31 2010
> @@ -8465,22 +8465,42 @@
>   MachineBasicBlock *sinkMBB = F->CreateMachineBasicBlock(LLVM_BB);
>   unsigned Opc =
>     X86::GetCondBranchFromCond((X86::CondCode)MI->getOperand(3).getImm());
> +
>   BuildMI(BB, DL, TII->get(Opc)).addMBB(sinkMBB);
>   F->insert(It, copy0MBB);
>   F->insert(It, sinkMBB);
> +
>   // Update machine-CFG edges by first adding all successors of the current
>   // block to the new block which will contain the Phi node for the select.
>   for (MachineBasicBlock::succ_iterator I = BB->succ_begin(),
>          E = BB->succ_end(); I != E; ++I)
>     sinkMBB->addSuccessor(*I);
> +
>   // Next, remove all successors of the current block, and add the true
>   // and fallthrough blocks as its successors.
>   while (!BB->succ_empty())
>     BB->removeSuccessor(BB->succ_begin());
> +
>   // Add the true and fallthrough blocks as its successors.
>   BB->addSuccessor(copy0MBB);
>   BB->addSuccessor(sinkMBB);
>
> +  // If the EFLAGS register isn't dead in the terminator, then claim that it's
> +  // live into the sink and copy blocks.
> +  const MachineFunction *MF = BB->getParent();
> +  const TargetRegisterInfo *TRI = MF->getTarget().getRegisterInfo();
> +  BitVector ReservedRegs = TRI->getReservedRegs(*MF);
> +  const MachineInstr *Term = BB->getFirstTerminator();
> +
> +  for (unsigned I = 0, E = Term->getNumOperands(); I != E; ++I) {
> +    const MachineOperand &MO = Term->getOperand(I);
> +    if (!MO.isReg() || MO.isKill() || MO.isDead()) continue;
> +    unsigned Reg = MO.getReg();
> +    if (Reg != X86::EFLAGS) continue;
> +    copy0MBB->addLiveIn(Reg);
> +    sinkMBB->addLiveIn(Reg);
> +  }
> +
>   //  copy0MBB:
>   //   %FalseValue = ...
>   //   # fallthrough to sinkMBB
>
> Modified: llvm/trunk/test/CodeGen/X86/MachineSink-CritEdge.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/MachineSink-CritEdge.ll?rev=106066&r1=106065&r2=106066&view=diff
> ==============================================================================
> --- llvm/trunk/test/CodeGen/X86/MachineSink-CritEdge.ll (original)
> +++ llvm/trunk/test/CodeGen/X86/MachineSink-CritEdge.ll Tue Jun 15 18:46:31 2010
> @@ -1,10 +1,4 @@
>  ; 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"
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>




More information about the llvm-commits mailing list