[PATCH] Fix write-back value propagation for pre-indexed addressing modes

[Silviu Baranga]

> -----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).

- 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.