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

[Sergei Larin]

I am late to the party :( 

...though I am glad it is clear now. I can go under anesthesia peacefully
today :) 

Sergei

---
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
The Linux Foundation


> -----Original Message-----
> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> bounces at cs.uiuc.edu] On Behalf Of Andrew Trick
> Sent: Monday, September 24, 2012 2:08 PM
> To: William J. Schmidt
> Cc: llvm commits
> Subject: Re: [llvm-commits] byval arg lowering (was: [PATCH, RFC] Fix
> PR13891 (AliasChain not properly maintained in
> ScheduleDAGInstrs::buildSchedGraph()))
> 
> 
> On Sep 24, 2012, at 12:00 PM, Andrew Trick <atrick at apple.com> wrote:
> 
> >
> > On Sep 24, 2012, at 11:47 AM, "William J. Schmidt"
> <wschmidt at linux.vnet.ibm.com> wrote:
> >
> >> On Mon, 2012-09-24 at 11:04 -0700, Andrew Trick wrote:
> >>> 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
> >>>
> >>
> >> OK, thanks for the prompt reply!  We'll work on this.
> >>
> >> It's still not quite clear to me what the AliasChain manipulations
> >> are doing at the moment.  Both objects end up being flagged as
> >> MayAlias, causing them to be added to the AliasChain.  Since the
> head
> >> of the AliasChain isn't updated, this results in them both being
> >> DAG'd to the compare at the end of the scheduling region, which
> >> essentially has no effect on anything.  Perhaps this is all WAD; it
> >> just seemed peculiar to me. ;)
> >
> >
> > That's right. The head of the AliasChain is a store to an unknown
> > location. Stores to known locations (as determined by
> > getUnderlyingObjectForInstr) effectively have their own chain. You
> have a load and store, both to known locations that appear to be
> different locations at the MachineMemOperand level.
> 
> I forgot to add... the MayAlias flag means that other unknown addresses
> may point to this known location. It does not mean that this known
> location may alias with other known locations. AFAIK, we have no way to
> express that two known locations (fixed stack slot vs. argument) may
> actually alias.
> 
> -Andy
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits