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

[Kyrill Tkachov <kyrylo dot tkachov at arm dot com>]

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?

Kyrill