> (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
