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

[Nick Lewycky]

On 01/05/2013 11:33 AM, Evan Cheng wrote:
>
> 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,

I'm afraid that breaks two tests. test/CodeGen/X86/fold-call-3.ll is 
expecting to see:

LBB0_2:                                 ## %bb
                                         ## =>This Inner Loop Header: 
Depth=1
	movq	(%rbx), %rax
	movq	%rbx, %rdi
	callq	*560(%rax)

but it now produces:

LBB0_2:                                 ## %bb
                                         ## =>This Inner Loop Header: 
Depth=1
	movq	(%rbx), %rax
	movq	560(%rax), %rax
	movq	%rbx, %rdi
	callq	*%rax

and test/CodeGen/X86/sibcall.ll which fails like so:

test/CodeGen/X86/sibcall.ll:232:7: error: expected string not found in input
; 64: jmpq *16({{%rdi|%rax}})
       ^
<stdin>:147:15: note: scanning from here
  movq 32(%rdi), %rdi
               ^
<stdin>:148:2: note: possible intended match here
  movq 16(%rdi), %rax
  ^

Both of these look like true positives.

I'm sure you've got the right idea with checking whether anything in the 
chain has a potentially aliasing memory write, but I don't understand 
the DAG well enough to write reasonable replacement code. I don't know 
what a tokenfactor is, for starters, and I don't know why we would or 
wouldn't exclude it.

Nick