[PATCH] Fix write-back value propagation for pre-indexed addressing modes
Hal Finkel
hfinkel at anl.gov
Thu Apr 25 12:57:43 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, "Dmitry Antipov" <antipov at dev.rtsoft.ru>
> Sent: Thursday, April 25, 2013 1:36:44 PM
> Subject: RE: [PATCH] Fix write-back value propagation for pre-indexed addressing modes
>
> Hi Hal,
>
> I have inlined my comments.
> ________________________________________
> From: Hal Finkel [hfinkel at anl.gov]
> Sent: 25 April 2013 17:47
> To: Silviu Baranga
> Cc: llvm-commits at cs.uiuc.edu; Dmitry Antipov
> Subject: Re: [PATCH] Fix write-back value propagation for pre-indexed
> addressing modes
>
>
> > 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;
>
> OK. The transformation that we are interested in starts from this
> point.
Alright, well CombineToPreIndexedLoadStore starts with the state above. While the code in question happens here it uses the Offset value determined above (which is why I thought that the starting point is relevant).
> Offset here is equal to c1 (or to be more precise the second
> operand of the address)
Right, and this is what I would have thought. Dmitry said that it was -c1, and this obviously makes a difference.
> and BasePtr is equal to p0 (the fist operand
> of the address of the new load/store instruction). We know that to
> be the case
> because of the following code:
>
> + SDValue Result;
> + if (isLoad)
> + Result = DAG.getIndexedLoad(SDValue(N,0), N->getDebugLoc(),
> + BasePtr, Offset, AM);
> + else
> + Result = DAG.getIndexedStore(SDValue(N,0), N->getDebugLoc(),
> + BasePtr, Offset, AM);
>
> We don't care about what the DAG looked like before because it is
> semantically equivalent with what we have now.
I understand; we're kind of talking past each other at this point. In any case, looking at the ARM implementation, I disagree with Dmitry's interpretation of the problem. As I wrote the code in question, please feel free to commit your fix.
I would really like, however, if we had a test case for this. If you had code that the current implementation miscompiled, can you place an assert that triggers in the bad case (pre_dec + swap), and use bugpoint to reduce the test case?
Thanks again,
Hal
>
> >
> > 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?
> Offset at this point is modified to be one of the operands in the
> address part of the newly generated load/store instruction. If we
> have a PRE_DEC and offset is the second operand, you just subtract
> that value. It doesn't really matter what the selection operation
> did.
>
> >
> > 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?
> I think this actually a case of generating a fix that works for this
> test case based on the wrong hypothesis.
>
> Cheers,
> Silviu
>
> >
> > Thanks again,
> > Hal
>
>
> ________________________________________
> From: Hal Finkel [hfinkel at anl.gov]
> Sent: 25 April 2013 17:47
> To: Silviu Baranga
> Cc: llvm-commits at cs.uiuc.edu; Dmitry Antipov
> 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: 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.
> >
>
> -- 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