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

[Hal Finkel]

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

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