[llvm-commits] patch: don't sink call address below store

[Evan Cheng]

On Jan 5, 2013, at 10:37 AM, Nick Lewycky <nicholas at mxc.ca> wrote:

> The attached patch fixes PR14739. It's not clear to me that this fix is actually the right fix as opposed to just covering up the bug for the one testcase I've got -- the function introducing the miscompile deliberately ignores the (correct) chain information and decides it knows better. That seems awfully dubious, but many tests fail if I try to just delete it outright.
> 
> I also don't know the SDNode hierarchy well enough to know if I could make this test smarter, such as by checking for "isa<LoadSDNode>" instead.

The patch looks right to me but it should also return false when its chain is a tokenfactor. Something like this?

  if (!Chain.getNumOperands())
    return false;
  // Since we are not checking for AA here, conservatively abort if the chain                                                                                                          
  // writes to memory. It's not safe to move the callee (a load) across a store.                                                                                                       
  if (isa<MemSDNode>(Chain.getNode()) &&
      cast<MemSDNode>(Chain.getNode())->writeMem())
    return false;
  if (Chain.getOperand(0).getNode() == Callee.getNode())
    return true;

Thanks,

Evan

> 
> Please review!
> 
> Nick
> <pr14739-1.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits