[PATCH] Fix write-back value propagation for pre-indexed addressing modes
Silviu Baranga
Silviu.Baranga at arm.com
Thu Apr 25 11:36:44 PDT 2013
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. Offset here is equal to c1 (or to be more precise the second operand of the address) 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.
>
> 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