[llvm-commits] byval arg lowering (was: [PATCH, RFC] Fix PR13891 (AliasChain not properly maintained in ScheduleDAGInstrs::buildSchedGraph()))

[Andrew Trick]

On Sep 21, 2012, at 1:34 PM, William J. Schmidt <wschmidt at linux.vnet.ibm.com> wrote:

> PR13891 identifies a test case where a load and store that reference the
> same memory location may be improperly interchanged.  Investigation
> showed this was due to a lost aliasing dependence.  If the only
> dependence between two instructions is a may-alias via the AliasChain,
> that dependence may be missed because the head of the AliasChain is not
> always properly updated.  This patch fixes that.
> 
> There was one regression failure in test/CodeGen/ARM/vstlane.ll.  I
> found that the code gen was identical before and after the patch, modulo
> one change in assigned registers.  I modified the test to be less
> dogmatic about which register is assigned in this case.
> 
> test/CodeGen/PowerPC/pr13891.ll is a new test that verifies the fix.
> 
> No additional regressions in llvm/tests or in llvm/projects/test-suite.
> Is this OK to commit?


Hi Bill,

The problem here is that the PowerPC machine code tells the scheduler that the load and store are two independent objects: one being an argument, and another being a local stack slot:

	STH8 %X3<kill>, 162, %X1; mem:ST2[FixedStack-1]
	%R3<def> = LHA 162, %X1; mem:LD2[%0]

In general, we don't want these kind of memory references to be serialized using the AliasChain.

MIPS had a similar problem: http://llvm.org/pr12205

I'm honestly not sure how to best handle this. My suggestion for Mips was to fix byval lowering to generate a MachinePointerInfo that refers to the argument value and offset of the store. That way, the stack initialization will naturally alias with any loads from the byval argument. That's still my best suggestion.

If anyone else has ideas for the proper way to handle byval lowering, please chime in!

-Andy