This is the mail archive of the
mailing list for the GCC project.
Re: [RFD] Simplifying subregs in LRA
- From: Eric Botcazou <ebotcazou at adacore dot com>
- To: Matthew Fortune <Matthew dot Fortune at imgtec dot com>
- Cc: gcc at gcc dot gnu dot org, "Alan Modra (amodra at gmail dot com)" <amodra at gmail dot com>, Vladimir Makarov <vmakarov at redhat dot com>, "wmi at google dot com" <wmi at google dot com>
- Date: Fri, 03 Feb 2017 13:06:52 +0100
- Subject: Re: [RFD] Simplifying subregs in LRA
- Authentication-results: sourceware.org; auth=none
- References: <6D39441BF12EF246A7ABCE6654B0235380B55AD3@hhmail02.hh.imgtec.org>
> (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
> 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).