Re: [RFA:] Fix PR target/18701, address adjustment error withparadoxical mem subregs. (Disallow them, I say!)

[Hans-Peter Nilsson <hp at bitrange dot com>]

On Wed, 29 Dec 2004, Roger Sayle wrote:

> I'm sure you realize that paradoxical subregs of MEMs have special
> semantics on hosts that define LOAD_EXTEND_OP?  On targets, such as
> MMIX, the upper bits of the paradoxical subreg of a MEM are not
> undefined but are either zero or sign extended (for MMIX depending
> upon the -mzero-extend command line option).

Yes, I do realize that.  It's a design decision in the MMIX port
after all.  Hmm, it seems I never wrote down *exactly* why,
besides the comment now at mmix.md:714.  My recollection is that
besides the issue of having two identical move patterns for the
same insn, one specifying *_extend, it would mean I have to
allow for register operands for the *_extend case as well, and
that doesn't exist on MMIX, so that seemed worse.  I can't match
a zero- or sign_extend in the actual move pattern, so much
better to tell GCC what memory loads actually do.  Oh, and see
<URL:http://gcc.gnu.org/ml/gcc-patches/2002-04/msg00066.html>
and <URL:http://gcc.gnu.org/ml/gcc/2002-01/msg01882.html>,
specifically the passage with "Long term, we should get rid of
(subreg (mem))" :-) and the nice
<URL:http://gcc.gnu.org/ml/gcc/2002-01/msg01830.html>.

> This is particularly significant as GCC's MMIX backend is one of the
> few targets doesn't define explicit sign or zero extension patterns.
> Indeed, config/mmix/mmix.md doesn't even contain the string sign_extend
> or zero_extend, instead forcing the middle-end to synthesise these
> widening operations.

Why do you say "force", making it seem all negative and that?
LOAD_EXTEND_OP is there to allow GCC to make use of the
extending sematics of normal memory accesses when combining
and/or when GCC wants a sign/zero extend.  For explicitness of
RTL, I think GCC should be able to generate sign/zero_extend for
MEM (as per LOAD_EXTEND_OP) when it needs, and match it to any
MEM source using LOAD_EXTEND_OP.  It should not be used in
subreg games as happens now.  (Hm, grepping on LOAD_EXTEND_OP
makes me think that's not too far away; generating
sign/zero_extend does seem to happen in places, but the matching
in recog () doesn't happen.)

> Given these constraints (no sign_extend or zero_extend RTL support but
> a defined LOAD_EXTEND_OP) its perhaps not surprising that the middle-end
> and RTL optimizers make use of paradoxical MEM subregs at almost every
> "load" opportunity, to eliminate the left-shift/right-shift sequence
> that would otherwise be necessary for integer widening operations.

"Every load opportunity" is IMHO not a proper representation of
what's supposed to happen (as in, is documented to happen and
global maintainers have agreed to should happen).  Paradoxical
MEM subregs are allowed only a limited period, "between the
combiner pass and the reload pass" (quote from docs).  See also
<URL:http://gcc.gnu.org/ml/gcc/2002-01/msg01806.html> (all
quotes in this message is from same thread, extended by
reference from later threads).

LOAD_EXTEND_OP-only-targets are not helped by those paradoxical
mem subregs.  Those subregs should never be.

> Perhaps "disallow them I say!" is a bit strong, but most targets are
> able to minimize or eliminate the use of paradoxical subregs of MEMs
> by adding  suitable sign and/or zero extension variants of thier load
> instructions or valid addressing modes :>

You see nothing wrong with those "variants"?  Doubling or
tripling the description of the exact same actual instruction,
when LOAD_EXTEND_OP should have expressed the "extending" load
semantics?  Besides, incorrect code paths in GCC should be
removed, not papered over by target definitions.  (By use of the
smiley, I can't tell if the whole paragraph was actually meant
as a joke.  I'll just say that paradoxical subregs is no joking
matter! 8-)

> On Tue, 21 Dec 2004, Hans-Peter Nilsson wrote:
> > Second, admittedly a bit simplistic, but seems valid looking at the other
> > things disallowed in that context:
> >
> > 	PR target/18701
> > 	* combine.c (combine_simplify_rtx): Do not allow paradoxical
> > 	subregs of MEM.
>
> Having said all that, I completely agree with your second patch that
> combine shouldn't be widening the mode used to access MEMs.  This is
> OK for mainline, however I think something more descriptive/accurate
> for the ChangeLog is on order.  Perhaps...
>
> 	PR target/18701
> 	* combine (combine_simplify_rtx): Do not widen the mode used
> 	to access the MEM of a paradoxical subreg.

The patch does really disallow a paradoxical subregs as a
simplification; the definition of "paradoxical subreg" for
memory is that the subreg widens the access of the original mem.
Your rewording of the ChangeLog is then confusing.

> Is this patch sufficient to resolve PR18701 on its own?

Yes, but... you're approving the papering-over part of the
patch?  Why not the address-calculation-for-stripped-subreg
correction (at least "in addition")?  I thought that one almost
obvious.  What was wrong with it?  If you think it's not needed
with the second (approved?) patch, then please remember that
this code is far from static, and a later patch may expose the
buggy execution path again.

And what is your take on the paradoxical mem difference for
INSN_SCHEDULING?  (That's reason why MMIX gets into this trouble
rather than the LOAD_EXTEND_OP matter.)

brgds, H-P