[llvm-commits] byval arg lowering (was: [PATCH, RFC] Fix PR13891 (AliasChain not properly maintained in ScheduleDAGInstrs::buildSchedGraph()))
Akira Hatanaka
ahatanak at gmail.com
Fri Sep 28 11:59:37 PDT 2012
Sergei,
I don't have any code that I can produce using llc. mips doesn't do tail
call optimization yet.
I was misunderstanding the problem when I asked the question, but I wanted
to know what kind of measure is taken to ensure the incoming argument area
isn't overwritten when a tail call writes its outgoing arguments to the
stack before the original value is read. This is a more general question
which probably has nothing to do with how functions with byval arguments
are handled.
Using the following program as an example, if tail call optimization were
enabled for mips,
$ cat tail.c
int g1, g2;
typedef struct {
int a0, a1, a2, a3, a4;
} S1;
static int f2(S1 a) { ... }
int f1(S1 a) {
g2 = a.a4;
a.a4 = g1;
return f2(a);
}
the generated code might look like this:
$ cat tail.s
1. lw $2,%got(g1)($28)
2. lw $3,16($sp) // $3 = a.a4. read out a.a4.
3. lw $2,0($2) // $2 = g1
4. sw $2,16($sp) // store g1 to stack slot of a.a4.
5. lw $25,%got(f2)($28)
6. addiu $25,$25,%lo(f2)
7. lw $2,%got(g2)($28)
8. jr $25 // tail call
9. sw $3,0($2) // g2 = $3
Since store (instruction 4) writes to the same location as the earlier load
(instruction 2) reads from, we should make sure the schedulers know that
the underlying objects can possibly alias.
I am not sure whether the targets which currently implement tail call
optimization have this problem or what is done to prevent it, but I am
guessing setting the store's MachinePointerInfo.V to 0 after the node is
created will suffice.
On Thu, Sep 27, 2012 at 8:50 AM, Sergei Larin <slarin at codeaurora.org> wrote:
> ** **
>
> Akira-san,****
>
> ** **
>
> Those are good questions. Do you have some sort of exemplary code to
> illustrate it? We also had to plow through a similar issue with byval
> operands in Hexagon, but I have not seen any issues with tail call
> optimizations so far.****
>
> ** **
>
> Sergei.****
>
> ** **
>
> ---****
>
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted
> by The Linux Foundation****
>
> ** **
>
> *From:* Akira Hatanaka [mailto:ahatanak at gmail.com]
> *Sent:* Tuesday, September 25, 2012 5:11 PM
> *To:* Sergei Larin
> *Cc:* Andrew Trick; William J. Schmidt; llvm commits
>
> *Subject:* Re: [llvm-commits] byval arg lowering (was: [PATCH, RFC] Fix
> PR13891 (AliasChain not properly maintained in
> ScheduleDAGInstrs::buildSchedGraph()))****
>
> ** **
>
> For mips, the problem was fixed by making MachinePointerInfo refer to
> function argument + offset, which seemed to be the simplest way. There were
> also a couple other alternative ways suggested by Andy (I didn't try any of
> them, so I don't know whether there will be any problems):
>
> http://thread.gmane.org/gmane.comp.compilers.llvm.devel/48116/focus=48238
>
> I have two related questions:
>
> 1. Does function MachinePointerInfo with argument + offset work if the
> target does tail call optimization? It seems to me that functions with
> byval arguments require extra care to ensure loads/stores are serialized
> correctly, if you want to do tail call optimization. Function
> SelectionDAG::getStackArgumentTokenFactor assumes the nodes which load the
> incoming arguments have chain inputs which are output by the entry node.
> This is no longer true if you have store nodes that write to the incoming
> stack area.
>
> 2. Is it not possible to preprocess functions with byval arguments at the
> IR level and get rid of byval arguments altogether by the time the backend
> sees the function? clang does some of that in CodeGen/TargetInfo.cpp. If
> the preprocessing is done later (but before isel/codegen), the code which
> is needed to handle byval args can be simplified or completely removed.
>
> ****
>
> On Tue, Sep 25, 2012 at 9:45 AM, Sergei Larin <slarin at codeaurora.org>
> wrote:****
>
> 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
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits****
>
> ** **
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120928/c9724918/attachment.html>
More information about the llvm-commits
mailing list