This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH][ARM]Replace gen_rtx_PLUS with plus_constant
- From: Richard Earnshaw <rearnsha at arm dot com>
- To: Kyrill Tkachov <kyrylo dot tkachov at arm dot com>
- Cc: Renlin Li <renli dot li at arm dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, Ramana Radhakrishnan <Ramana dot Radhakrishnan at arm dot com>
- Date: Mon, 23 Sep 2013 15:45:46 +0100
- Subject: Re: [PATCH][ARM]Replace gen_rtx_PLUS with plus_constant
- Authentication-results: sourceware.org; auth=none
- References: <523C5BEE dot 2090509 at arm dot com> <5240452C dot 4050705 at arm dot com>
On 23/09/13 14:42, Kyrill Tkachov wrote:
> Hi Renlin, all,
> On 20/09/13 15:30, Renlin Li wrote:
>> --- a/gcc/config/arm/arm.c
>> +++ b/gcc/config/arm/arm.c
>> @@ -7016,10 +7016,10 @@ arm_legitimize_reload_address (rtx *p,
>> /* Reload the high part into a base reg; leave the low part
>> in the mem. */
>> - *p = gen_rtx_PLUS (GET_MODE (*p),
>> - gen_rtx_PLUS (GET_MODE (*p), XEXP (*p, 0),
>> - GEN_INT (high)),
>> - GEN_INT (low));
>> + *p = plus_constant (GET_MODE (*p),
>> + plus_constant (GET_MODE (*p), XEXP (*p, 0),
>> + high),
>> + low);
>> push_reload (XEXP (*p, 0), NULL_RTX, &XEXP (*p, 0), NULL,
>> MODE_BASE_REG_CLASS (mode), GET_MODE (*p),
>> VOIDmode, 0, 0, opnum, (enum reload_type) type);
> We have to be careful with this hunk. plus_constant performs automatic constant
> folding on its arguments, whereas gen_rtx_PLUS does not. This means that with
> this patch, *p will be of the form:
> (plus (reg) (const))
> while currently it is:
> (plus (reg)
> While constant folding is in general a good thing, in this case the next line
> (push_reload) accesses XEXP (*p, 0) which is now different between the two
> revisions. This is a similar bug to what was in your AArch64 patch
> (http://gcc.gnu.org/ml/gcc-patches/2013-09/msg01536.html ) that caused
> regressions there.
> Therefore I suggest this hunk is removed from the patch.
> What do the maintainers think?
Yes, that makes sense. We could probably use plus_constant for the
innermost operation, but we certainly can't for the outer part. In
practice it probably is best not to use it for either.
Please can you add a comment to that effect so that we don't try to make
the same mistake again...