Eric Botcazou <ebotca...@adacore.com> writes:
> > (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?
It's not a no-op. Any MEM_P not handled by the first "if (MEM_P(reg))"
will have previously been handled by the block guarded by the following
later in the function; note the "|| MEM_P (reg)":
/* Force a reload of the SUBREG_REG if this is a constant or PLUS or
if there may be a problem accessing OPERAND in the outer
mode. */
if ((REG_P (reg)
&& REGNO (reg) >= FIRST_PSEUDO_REGISTER
&& (hard_regno = lra_get_regno_hard_regno (REGNO (reg))) >= 0
/* Don't reload paradoxical subregs because we could be looping
having repeatedly final regno out of hard regs range. */
&& (hard_regno_nregs[hard_regno][innermode]
>= hard_regno_nregs[hard_regno][mode])
&& simplify_subreg_regno (hard_regno, innermode,
SUBREG_BYTE (operand), mode) < 0
/* Don't reload subreg for matching reload. It is actually
valid subreg in LRA. */
&& ! LRA_SUBREG_P (operand))
|| CONSTANT_P (reg) || GET_CODE (reg) == PLUS || MEM_P (reg))
{
> > 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.
I'm not sure what check to do on mode sizes. Do you think an innermode
reload is only required when both modes have the same number of words?
> > 2) [BUG] Remove the return false introduced in (r243782 git:856bd6f).
>
> !???
If a MEM subreg is neither simplified to an outermode MEM nor reloaded
in innermode then I believe LRA will never resolve the subreg. Even if that
is not true I'm fairly certain the addition of the code has changed
behaviour and that the change is not well understood, as explained above.
> > 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.
Indeed, definitely want to wait for GCC 8.
> > 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).
I hadn't spotted that table, very helpful, thanks. The other architectures
I listed may be helped in their transition to LRA. I guess they are
attempting to move given optional LRA support.
Thanks,
Matthew
