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

[Silviu Baranga]

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.

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