Re: paradoxical subreg problem [ patch attached ]

[Jim Wilson <wilson at cygnus dot com>]

>[ Kenner, does this look familiar :-) ]

Kenner's original patch was added in 1991, a couple of months after he
completely rewrote combine.c.  There was a lot of change in combine.c at
the time, and it is very unlikely anyone will remember all of the details
of any one particular change.

Kenner's change is correct if you exclude (subreg (mem)).  The (subreg (mem))
meaning changed as part of Kenner's combine.c rewrite, as this is when
BYTE_LOADS_ZERO_EXTEND was introduced.  BYTE_LOADS_ZERO_EXTEND was the
forerunner of the current WORD_REGISTER_OPERATIONS and LOAD_EXTEND_OP macros.
I believe Kenner's change will correct again in the future after we get rid
of (subreg (mem)).  But for now, it is wrong.  It looks like a simple
oversight.  Or maybe BYTE_LOADS_ZERO_EXTEND was so new at the time that we
had not yet realized that the meaning of (subreg (mem)) had (accidentally?)
changed.  Or maybe the (subreg (mem)) confusion happened later.

>	* combine.c (simplify_comparison): Avoid narrowing a comparison
>	with a paradoxical subreg when doing so would drop signficant bits.

This patch looks reasonable to me.  Excluding (subreg (mem)) should do the
right thing.  You might want to add a comment pointing to LOAD_EXTEND_OP
and/or WORD_REGISTER_OPERATIONS so that this extra check can be deleted if/when
(subreg (mem)) goes away.

Long term, we should get rid of (subreg (mem)).  Looking at Alan Modra's
comments in PR 5169, it looks like the problem in this specific case is an
accidental bad interaction in combine RTL simplification.  We started with
a zero_extend, converted it to two shifts, converted them to an AND, and
then optimized away the AND because nonzero_bits knows that (subreg (mem))
has a special meaning, leaving us with (subreg (mem)) which is undesirable.
We should either stop expand_compound_operation from converting zero_extend
to subreg, which is what Alan Modra's patch does, or else we should fix
nonzero_bits to stop handling (subreg (mem)) specially.  The latter is risky
since it might result in lost optimizations.  The former seems reasonably safe,
but not as safe as your change, since Alan's patch changes how we canonicalize
RTL inside combine, but your patch only diasables a specific RTL rewrite that
is unsafe.  Alan's patch has the advantage that it might fix more instances
of this problem, since your patch only fixes one specific case.  But perhaps
this is the only specific case at the moment that is broken.  I think we
would want Alan's patch if/when we eliminate (subreg (mem)) though.  Or maybe
a revised version of Alan's patch that aborts if we think this RTL should
never be created at this point.  That depends on whether the nonzero_bits
handling of (subreg (mem)) is doing anything useful.

Jim