Hi Eric, Any thoughts on this?
Thanks, Matthew > Sorry for the slow reply, been away for a few days.... > > Eric Botcazou <ebotca...@adacore.com> writes: > > > This patch is a minimal change to prevent (subreg(mem)) from being > > > simplified to use the outer mode for WORD_REGISTER_OPERATIONS. > > > There is high probability of refining and/or re-implementing this > > > for GCC 8 but such a change would be too invasive. This change at > > > least ensures correctness but may prevent simplification of some > acceptable cases. > > > > This one causes: > > > > +FAIL: gcc.dg/torture/builtin-complex-1.c -O3 -fomit-frame-pointer - > > funroll- > > loops -fpeel-loops -ftracer -finline-functions (test for excess > > errors) > > +WARNING: gcc.dg/torture/builtin-complex-1.c -O3 -fomit-frame- > pointer > > - > > funroll-loops -fpeel-loops -ftracer -finline-functions compilation > > failed to produce executable > > +FAIL: gcc.dg/torture/builtin-complex-1.c -O3 -g (test for excess > > errors) > > +WARNING: gcc.dg/torture/builtin-complex-1.c -O3 -g compilation > > failed to > > produce executable > > +WARNING: program timed out. > > +WARNING: program timed out. > > > > on SPARC 32-bit, i.e. LRA hangs. Reduced testcase attached, compile > > at > > -O3 with a cc1 configured for sparc-sun-solaris2.10. > > > > > gcc/ > > > PR target/78660 > > > * lra-constraints.c (simplify_operand_subreg): Handle > > > WORD_REGISTER_OPERATIONS targets. > > > --- > > > gcc/lra-constraints.c | 17 ++++++++++++----- > > > 1 file changed, 12 insertions(+), 5 deletions(-) > > > > > > diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c index > > > 66ff2bb..484a70d 100644 > > > --- a/gcc/lra-constraints.c > > > +++ b/gcc/lra-constraints.c > > > @@ -1541,11 +1541,18 @@ simplify_operand_subreg (int nop, > > > machine_mode > > > reg_mode) subregs as we don't substitute such equiv memory (see > > > processing 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)))) > > > + corresponding paradoxical subreg. > > > + > > > + However, never simplify a (subreg (mem ...)) for > > > + WORD_REGISTER_OPERATIONS targets as this may lead to loading > > junk > > > + data into a register when the inner is narrower than outer or > > > + missing important data from memory when the inner is wider > > than > > > + outer. */ > > > + 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; > > > > I think that we might need: > > > > if (!(GET_MODE_PRECISION (mode) > GET_MODE_PRECISION (innermode) > > && 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; > > > > i.e. we force the reloading only for paradoxical subregs. > > Maybe, though I'm not entirely convinced. For WORD_REGISTER_OPERATIONS > both paradoxical and normal SUBREGs can be a problem as the inner mode > in both cases can be used elsewhere for a reload making the content of > the spill slot wrong either in the subreg reload or the ordinary reload > elsewhere. However, the condition can be tightened; I should not have > made it so simplistic I guess. I.e. both modes must be no wider than a > word and must be different precision to force an inner reload. > > Adding that check would fix this case for SPARC and should be fine for > MIPS but I need to wait for a bootstrap build to be sure. > > I don't really understand why LRA can't reload this for SPARC though as > I'm not sure there is any guarantee provided to backends that some > SUBREGs will be reloaded using their outer mode. If there is such a > guarantee then it would be much easier to reason about this logic but as > it stands I suspect we are having to tweak LRA to cope with assumptions > made in various targets that happen to have held true (and I have no > doubt that MIPS has some of these as well especially in terms of the > FP/GP register usage with float and int modes.) All being well we can > capture such assumptions and formalise them so we ensure they hold true > (or modify backends appropriately I guess). > > The condition would look like this, What do you think? > > if (!(GET_MODE_PRECISION (mode) != GET_MODE_PRECISION > (innermode) > && GET_MODE_SIZE (mode) <= UNITS_PER_WORD > && GET_MODE_SIZE (innermode) <= UNITS_PER_WORD > && 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; > > Thanks, > Matthew