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

[Ulrich Weigand]

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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: diff-llvm-dynalloc-csefix
Type: application/octet-stream
Size: 5389 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121108/b594f6ec/attachment.obj>