[llvm-commits] [PATCH] Fix MachineCSE bug causing miscompile of oggenc on PowerPC

[Evan Cheng]

LGTM

Evan
On Nov 8, 2012, at 6:03 AM, Ulrich Weigand <Ulrich.Weigand at de.ibm.com> wrote:

> 
> Hello,
> 
> as described in this email a while back:
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20121008/152920.html
> the oggenc test case is currently miscompiled on PowerPC, because
> MachineCSE merges two DYNALLOC8 nodes representing two separate alloca
> calls into a single node:
> 		 %vreg5<def> = DYNALLOC8 %vreg4, 0, <fi#-1>, %X1<imp-def,dead>,
> %X1<imp-use>; G8RC:%vreg5,%vreg4
> 		 %vreg6<def> = DYNALLOC8 %vreg4, 0, <fi#-1>, %X1<imp-def,dead>,
> %X1<imp-use>; G8RC:%vreg6,%vreg4
> into
> 		 %vreg5<def> = DYNALLOC8 %vreg4, 0, <fi#-1>, %X1<imp-def,dead>,
> %X1<imp-use>; G8RC:%vreg5,%vreg4
> 
> In the email back in October, I noted that this problem disappears when
> marking the DYNALLOC8 node as "mayStore".  However, there seemed to be some
> concern that this would just paper over the problem, and in fact even
> without mayStore, CSE ought not to merge these two nodes.
> 
> Thus I went back and had a closer look at the CSE algorithm.  The code does
> in fact notice that %X1 is used by the node, and it attempts to verify that
> this is still safe:
> 
>    // If the instruction defines physical registers and the values *may*
> be
>    // used, then it's not safe to replace it with a common subexpression.
>    // It's also not safe if the instruction uses physical registers.
>    bool CrossMBBPhysDef = false;
>    SmallSet<unsigned, 8> PhysRefs;
>    SmallVector<unsigned, 2> PhysDefs;
>    if (FoundCSE && hasLivePhysRegDefUses(MI, MBB, PhysRefs, PhysDefs)) {
>      FoundCSE = false;
> 
>      // ... Unless the CS is local or is in the sole predecessor block
>      // and it also defines the physical register which is not clobbered
>      // in between and the physical register uses were not clobbered.
>      unsigned CSVN = VNT.lookup(MI);
>      MachineInstr *CSMI = Exps[CSVN];
>      if (PhysRegDefsReach(CSMI, MI, PhysRefs, PhysDefs, CrossMBBPhysDef))
>        FoundCSE = true;
>    }
> 
> 
> So the code tries to check that "the physical register uses were not
> clobbered" in PhysRegDefsReach.   However, it does that by scanning all
> instructions *between* the first and second DYNALLOC8 node, and checking
> that none of those modifies %X1.  This is in fact trivially true, since
> there are no instructions between those two.  What the code neglects to
> notice is that the first DYNALLOC8 node itself already modifies %X1 ...
> 
> So I thought to modify PhysRegDefsReach to include that initial instruction
> in its scan.  And this does indeed prevent those two DYNALLOC8 instructions
> being merged.  Unfortunately, it prevents a lot more: since "PhysRefs"
> above holds not just physical registers used, but also physical registers
> *defined* by the instruction, *any* instruction that defines a physical
> register now no longer can be CSE'd.  This is not what we want (e.g. it
> prevent any instruction that sets %eflags from being CSE'd).
> 
> What we really want is to distinguish between physical register defs and
> uses:  for a use in MI, a def by CSMI is problematic, but for a def in MI,
> it is not.  Unfortunately, as defs and uses are mixed up in PhysRefs and
> cannot be separated out any more, this is not really feasible to do in
> PhysRegDefsReach.
> 
> On the other hand, since CSMI is a copy of MI anyway, we can check an
> equivalent simpler condition: if MI both defines and uses the same physical
> register, it is not suitable for CSE.   This condition can in fact be
> checked in hasLivePhysRegDefUses already; we do not need to walk any
> instructions to find out.
> 
> The attached patch implements this check.  This fixes the oggenc test case,
> and does not introduce any regressions in the full test suite (e.g. due to
> missed optimizations) either.
> 
> OK to commit?
> 
> Bye,
> Ulrich
> 
> (See attached file: diff-llvm-dynalloc-csefix)<diff-llvm-dynalloc-csefix>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits