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.
