[RELOAD] Don't assume subreg mem address is ok

On Mon, Aug 08, 2016 at 12:55:59PM +0930, Alan Modra wrote:
> On Mon, Aug 08, 2016 at 12:13:18PM +0930, Alan Modra wrote:
> > This patch fixes a case where reload blindly assumes a subreg mem
> > is OK if its address has been partially reloaded by
> > legitimize_reload_address.  The change ought to be fairly obvious
> > considering that the analogous non-subreg case only gives a win to
> > address_reloaded[i] == 1.  Bootstrapped and regression tested
> > powerpc64le-linux using
> >   RUNTESTFLAGS="--target_board=unix'{-mlra,-mno-lra}'"
> > to run the testsuite using both lra and reload.  OK for mainline
> > and active branches?
> 
> When I look at the correct bootstrap I see I have some regressions.
> Sorry for the noise, patch withdrawn.

Hi Ulrich, Bernd,
It turns out that the patch just exposed an existing rs6000 backend
problem, that made the result of forcing a constant to memory invalid
even for the "m" constraint.  So please consider the patch again.

Segher, is this rs6000 patch OK?  Bootstrapped as above.

	PR target/72771
	* config/rs6000/rs6000.c (toc_relative_expr_p): Allow lo_sum/high
	toc refs created during reload.

On Mon, Aug 08, 2016 at 05:03:46PM +0930, Alan Modra wrote:
> Segher, is this rs6000 patch OK?  Bootstrapped as above.
> 
> 	PR target/72771
> 	* config/rs6000/rs6000.c (toc_relative_expr_p): Allow lo_sum/high
> 	toc refs created during reload.

>  /* Return true if OP is a toc pointer relative address (the output
> -   of create_TOC_reference).  If STRICT, do not match high part or
> -   non-split -mcmodel=large/medium toc pointer relative addresses.  */
> +   of create_TOC_reference).  If STRICT, do not match non-split
> +   -mcmodel=large/medium toc pointer relative addresses.  */

Why this comment change?  The code still does not allow high part if
strict?  The changelog says it does now, is the code correct?


Segher


>  bool
>  toc_relative_expr_p (const_rtx op, bool strict)
> @@ -7949,13 +7949,17 @@ toc_relative_expr_p (const_rtx op, bool strict)
>  
>    if (TARGET_CMODEL != CMODEL_SMALL)
>      {
> -      /* Only match the low part.  */
> -      if (GET_CODE (op) == LO_SUM
> -	  && REG_P (XEXP (op, 0))
> -	  && INT_REG_OK_FOR_BASE_P (XEXP (op, 0), strict))
> -	op = XEXP (op, 1);
> -      else if (strict)
> +      /* When strict ensure we have everything tidy.  */
> +      if (strict
> +	  && !(GET_CODE (op) == LO_SUM
> +	       && REG_P (XEXP (op, 0))
> +	       && INT_REG_OK_FOR_BASE_P (XEXP (op, 0), strict)))
>  	return false;
> +
> +      /* When not strict, allow non-split toc addresses and also allow
> +	 lo_sum/high addresses created during reload.  */
> +      if (GET_CODE (op) == LO_SUM)
> +	op = XEXP (op, 1);
>      }

On Mon, Aug 08, 2016 at 03:52:31AM -0500, Segher Boessenkool wrote:
> On Mon, Aug 08, 2016 at 05:03:46PM +0930, Alan Modra wrote:
> > Segher, is this rs6000 patch OK?  Bootstrapped as above.
> > 
> > 	PR target/72771
> > 	* config/rs6000/rs6000.c (toc_relative_expr_p): Allow lo_sum/high
> > 	toc refs created during reload.
> 
> >  /* Return true if OP is a toc pointer relative address (the output
> > -   of create_TOC_reference).  If STRICT, do not match high part or
> > -   non-split -mcmodel=large/medium toc pointer relative addresses.  */
> > +   of create_TOC_reference).  If STRICT, do not match non-split
> > +   -mcmodel=large/medium toc pointer relative addresses.  */
> 
> Why this comment change?  The code still does not allow high part if
> strict?  The changelog says it does now, is the code correct?

The code is now correct, and allows (lo_sum (high (...)) style
addresses created during reload when non-strict.  That's what I meant
by the ChangeLog entry.

I felt that saying "do not match high" in the function comment is
extraneous as one form of the non-split insn is
(lo_sum (high (unspec [sym r2] UNSPEC_TOCREL))
        (unspec [sym r2] UNSPEC_TOCREL))
The other is plain (unspec [sym r2] UNSPEC_TOCREL).

On Mon, Aug 08, 2016 at 07:15:01PM +0930, Alan Modra wrote:
> > > Segher, is this rs6000 patch OK?  Bootstrapped as above.
> > > 
> > > 	PR target/72771
> > > 	* config/rs6000/rs6000.c (toc_relative_expr_p): Allow lo_sum/high
> > > 	toc refs created during reload.
> > 
> > >  /* Return true if OP is a toc pointer relative address (the output
> > > -   of create_TOC_reference).  If STRICT, do not match high part or
> > > -   non-split -mcmodel=large/medium toc pointer relative addresses.  */
> > > +   of create_TOC_reference).  If STRICT, do not match non-split
> > > +   -mcmodel=large/medium toc pointer relative addresses.  */
> > 
> > Why this comment change?  The code still does not allow high part if
> > strict?  The changelog says it does now, is the code correct?
> 
> The code is now correct, and allows (lo_sum (high (...)) style
> addresses created during reload when non-strict.  That's what I meant
> by the ChangeLog entry.

Aha.  "lo_sum of high", perhaps, although that also does not make much
sense unless you already know what it is talking about.  I read "/" as
"or".

> I felt that saying "do not match high" in the function comment is
> extraneous as one form of the non-split insn is
> (lo_sum (high (unspec [sym r2] UNSPEC_TOCREL))
>         (unspec [sym r2] UNSPEC_TOCREL))
> The other is plain (unspec [sym r2] UNSPEC_TOCREL).

It said that it does not allow just  (high ...)  I think.  There of
course are lots of other things it does not match ;-)

Okay for trunk.  Please note in the changelog that you changed the comment.

Thanks,


Segher