Re: [RFD] Simplifying subregs in LRA

[Eric Botcazou <ebotcazou at adacore dot com>]

> (r243782 git:856bd6f)
> This is another case of multiple changes where some were not critical and
> overall there is a dangerous one here I believe.  The primary aim of this
> change is to reload the address before reloading the inner subreg.  This
> appears to be a dormant bug since day1 as the original logic would have
> failed when reloading an inner mem if its address was not already valid.
> 
> The potentially fatal part of this change is the introduction of a
> "return false" in the MEM subreg simplification which is placed immediately
> after restoring the original subreg expression.  I believe that if control
> ever actually reaches this statement then LRA would infinite loop as the
> MEM subreg would never be simplified. 

How can a change that is a no-op be fatal exactly?

> So what are the next steps!
> 
> 1) [BUG] Add an exclusion for WORD_REGISTER_OPERATIONS because MIPS is
> currently broken by the existing code. PR78660

That seems the way to go, with the appropriate check on the mode sizes.

> 2) [BUG] Remove the return false introduced in (r243782 git:856bd6f).

!???

> 3) [CLEANUP] Remove reg_mode argument and replace all uses of reg_mode with
>    innermode.  Rename 'reg' to 'inner' and 'operand' to 'outer' and 'mode'
> to 'outermode'.
> 4) [OPTIMISATION] Change double-reload logic so that it just deals with the
>    special outermode reload without adjusting the subreg.
> 5) [??] Determine if big endian still needs a special case like in reload?
>    Comments anyone?

I agree that a cleanup of the code would probably be in order, with an eye on 
the reload code as a model, but that's probably not appropriate for GCC 7.

> In an attempt to make a minimal change I propose the following as it allows
> WORD_REGISTER_OPERATIONS targets to benefit from the invalid address
> reloading fix. I think the check would be more appropriately placed on the
> outer-most if (MEM_P (reg)) but this would affect the handling of many more
> subregs which seems too dangerous at this point in release.
> 
> diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
> index 393cc69..771475a 100644
> --- a/gcc/lra-constraints.c
> +++ b/gcc/lra-constraints.c
> @@ -1512,10 +1512,11 @@ simplify_operand_subreg (int nop, machine_mode
> reg_mode) equivalences in function lra_constraints) and because for spilled
> pseudos we allocate stack memory enough for the biggest
>  	     corresponding paradoxical subreg.  */
> -	  if (!(MEM_ALIGN (subst) < GET_MODE_ALIGNMENT (mode)
> -		&& SLOW_UNALIGNED_ACCESS (mode, MEM_ALIGN (subst)))
> -	      || (MEM_ALIGN (reg) < GET_MODE_ALIGNMENT (innermode)
> -		  && SLOW_UNALIGNED_ACCESS (innermode, MEM_ALIGN (reg))))
> +	  if (!WORD_REGISTER_OPERATIONS
> +	      && (!(MEM_ALIGN (subst) < GET_MODE_ALIGNMENT (mode)
> +		    && SLOW_UNALIGNED_ACCESS (mode, MEM_ALIGN (subst)))
> +		  || (MEM_ALIGN (reg) < GET_MODE_ALIGNMENT (innermode)
> +		      && SLOW_UNALIGNED_ACCESS (innermode, MEM_ALIGN 
(reg)))))
>  	    return true;
> 
>  	  *curr_id->operand_loc[nop] = operand;
> 
> 
> The change will affect at least arc,mips,rx,sh,sparc though I haven't
> checked which of these default on for LRA just that they can turn on LRA.

Only MIPS and SPARC (see https://gcc.gnu.org/backends.html).

-- 
Eric Botcazou