[PATCH] Fix write-back value propagation for pre-indexed addressing modes
Hal Finkel
hfinkel at anl.gov
Thu Apr 25 09:47:23 PDT 2013
----- Original Message -----
> From: "Silviu Baranga" <Silviu.Baranga at arm.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: llvm-commits at cs.uiuc.edu
> Sent: Thursday, April 25, 2013 11:07:09 AM
> Subject: RE: [PATCH] Fix write-back value propagation for pre-indexed addressing modes
>
>
>
> > -----Original Message-----
> > From: Hal Finkel [mailto:hfinkel at anl.gov]
> > Sent: 25 April 2013 16:27
> > To: Silviu Baranga
> > Cc: llvm-commits at cs.uiuc.edu
> > Subject: Re: [PATCH] Fix write-back value propagation for
> > pre-indexed
> > addressing modes
> >
> > ----- Original Message -----
> > > From: "Silviu Baranga" <Silviu.Baranga at arm.com>
> > > To: "Hal Finkel" <hfinkel at anl.gov>
> > > Cc: llvm-commits at cs.uiuc.edu
> > > Sent: Wednesday, April 24, 2013 3:42:31 PM
> > > Subject: RE: [PATCH] Fix write-back value propagation for
> > > pre-indexed
> > addressing modes
> > >
> > > Hi Hal,
> > >
> > > Thanks for pointing this out. I didn't know that there was
> > > already a
> > > bug related to this issue.
> > > I think that the regression test from Dmitry's work should be
> > > included.
> > >
> > > Semantically comparing the code from the patch with the existing
> > > code
> > > shows that there are
> > > some other cases that were not handled.
> > >
> > > Regarding your question: the 'OV' constant is obtained from a
> > > target
> > > hook
> > > (getPreIndexedAddressParts), so I don't think we can make any
> > > assumptions about it.
> >
> > Maybe I've confused myself, but I think that this semantic is part
> > of
> > the interface. If the target tells us that the instruction is pre-
> > decrement with a specific offset, OV, by which the result will be
> > decremented, then to combine this offset with other sourrounding
> > add/sub instructions, I'd think that we need to know whether the
> > pre-
> > decrement instruction is actually adding OV to the incoming pointer
> > value or subtracting it.
>
> As far as I understand, the PRE_DEC/PRE_INC only mean that we also
> get an output value equal to the address of the load and store
> operation,
> the only difference between them being that for PRE_DEC we subtract
> the second operand from the first, while for PRE_INC we add the first
> and
> second operands to get the address.
>
> I don't think there is actually a requirement for one of the two
> operands
> to be a constant since there are targets that have instructions where
> both operands can be registers (for example ARM). Therefore, we
> shouldn't
> be able to make any assertions about the sign of an operand if it
> happens
> to be a constant.
>
> I don't really understand why we need to know the sign for OV (maybe
> I'm missing
> something).
Okay, let's back up... Here's what we start with:
p0 : pointer;
p1 = p0 - c1;
l1 = load p1;
p2 = p0 + c2;
l2 = load p2;
Now, we call TLI.getPreIndexedAddressParts providing {l1 = load p1} and it returns BasePtr = {p0} and Offset = {-c1} with AM = ISD::PRE_DEC; My hypothesis is that it is important to know whether it will return Offset = {-c1} or Offset = {c1} with the PRE_DEC addressing mode. The core addressing-mode transformation does this:
p0 : pointer;
l1, p1 = load_pre_dec p0, c1;
p2 = p0 + c2;
l2 = load p2;
and what we'd really like, because c1 and c2 are constants, is this:
p0 : pointer;
l1, p1 = load_pre_dec p0, c1;
p2 = p1 + ( c1 + c2 );
l2 = load p2;
because doing it this way means that we don't need to keep an extra copy of p0 around after the pre-decrement load. In order to correctly compute the (c1 + c2) based on the value in Offset, I need to know whether Offset == c1 or Offset == -c1. In other words, I need to know whether the 'DEC' part of the PRE_DEC addressing mode is duplicated in the sign of the Offset or not. Does this make sense?
>From what Dmitry said, the reason that the current code is buggy is because I assumed that Offset == c1, but as the ARM backend implements it, Offset == -c1. Do you agree?
Thanks again,
Hal
>
> - Silviu
>
>
>
> >
> > -Hal
> >
> > >
> > > - Silviu
> > > ________________________________________
> > > From: llvm-commits-bounces at cs.uiuc.edu
> > > [llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Hal Finkel
> > > [hfinkel at anl.gov]
> > > Sent: 24 April 2013 19:16
> > > To: Silviu Baranga
> > > Cc: llvm-commits at cs.uiuc.edu
> > > Subject: Re: [PATCH] Fix write-back value propagation for
> > > pre-indexed
> > > addressing modes
> > >
> > > ----- Original Message -----
> > > > From: "Silviu Baranga" <Silviu.Baranga at arm.com>
> > > > To: llvm-commits at cs.uiuc.edu
> > > > Sent: Wednesday, April 24, 2013 11:02:01 AM
> > > > Subject: [PATCH] Fix write-back value propagation for
> > > > pre-indexed
> > > > addressing modes
> > > >
> > > > Hi,
> > > >
> > > > The attached patch fixes a bug in DAG combiner that causes a
> > > > wrong
> > > > usage
> > > > of the value write-back value obtained from pre-indexed
> > > > addressing
> > > > modes.
> > > >
> > > > The bug manifests itself when the base pointer and the offset
> > > > are
> > > > swapped
> > > > in a load/store store instruction with a pre decrement
> > > > addressing
> > > > mode.
> > > >
> > > > If a user needs to subtract the base from a constant value, the
> > > > current
> > > > code re-writes the expression as a SUB instruction, subtracting
> > > > the
> > > > write-back
> > > > value from an adjusted constant. This is wrong, since the
> > > > correct
> > > > output
> > > > is to add an adjusted constant to the write-back value.
> > > >
> > > > Since the code is apparently impossible to test, the patch does
> > > > not
> > > > contain a
> > > > regression test. Because of this I've re-written the code that
> > > > handles the
> > > > propagation of the write-back value so that it is easy to
> > > > follow
> > > > and
> > > > unlikely
> > > > to have fundamental bugs in it.
> > >
> > > Silviu,
> > >
> > > Thanks for working on this. This is PR15581, and Dmitry Antipov
> > > has
> > > proposed a simple fix (which seems to be covered by your code as
> > > well). I'll ask you the same question that I asked him:
> > >
> > > Is is safe to add something like this?
> > > assert(AM == ISD::PRE_DEC && OV <= 0 && "Offset for PRE_DEC must
> > > be
> > > negative");
> > >
> > > My misunderstanding of how the offsets for PRE_DEC addressing are
> > > interpreted is, from what I now understand from Dmitry, the cause
> > > of
> > > the current underlying problem.
> > >
> > > Regardless, I agree that your code is cleaner :)
> > >
> > > -Hal
> > >
> > > >
> > > > Please review!
> > > >
> > > > Thanks,
> > > > Silviu
> > > >
> > > > -- IMPORTANT NOTICE: The contents of this email and any
> > > > attachments
> > > > are confidential and may also be privileged. If you are not the
> > > > intended recipient, please notify the sender immediately and do
> > > > not
> > > > disclose the contents to any other person, use it for any
> > > > purpose,
> > > > or store or copy the information in any medium. Thank you.
> > > > _______________________________________________
> > > > 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
> > >
> > > -- IMPORTANT NOTICE: The contents of this email and any
> > > attachments
> > > are confidential and may also be privileged. If you are not the
> > > intended recipient, please notify the sender immediately and do
> > > not
> > > disclose the contents to any other person, use it for any
> > > purpose,
> > > or store or copy the information in any medium. Thank you.
> > >
> > >
>
>
> -- IMPORTANT NOTICE: The contents of this email and any attachments
> are confidential and may also be privileged. If you are not the
> intended recipient, please notify the sender immediately and do not
> disclose the contents to any other person, use it for any purpose,
> or store or copy the information in any medium. Thank you.
>
More information about the llvm-commits
mailing list