Re: [PATCH][ARM]Replace gen_rtx_PLUS with plus_constant

[Richard Earnshaw <rearnsha 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
>      (plus (reg)
>            (const1))
>      const2)
> 
> 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...

R.