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

[Evan Cheng]

I've fixed it with r171665. Thanks for looking at this.

Evan

On Jan 5, 2013, at 4:59 PM, Nick Lewycky <nicholas at mxc.ca> wrote:

> 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