Re: paradoxical subreg problem [ patch attached ]

[law at redhat dot com]

 In message <200201292139.NAA31158@wilson.cygnus.com>, Jim Wilson writes:
 > Kenner's change is correct if you exclude (subreg (mem)).
 [ ... ]
Agreed.

 > 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.
Agreed.

 > 
 > >	* 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.
Definitely needs comments.  Thanks for pointing that out.


 > Long term, we should get rid of (subreg (mem)).
I couldn't agree more.

 > 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.
Correct.  I skipped some of those conversions as they were completely
non-controversial (converting zero_extend into shifts, shifts into an AND)).

 > 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.
I was nearly ready to twiddle nonzero_bits until we decided that the
the high bits in a paradoxical (subreg (mem)) were known to be zero/one
when LOAD_EXTEND_OP is defined.

As for Alan's change to avoid zero_extend->subreg conversion, I decided
against it for the same reason.  It's safe and correct given the semantics
for paradoxical (subreg (mem)).


 > 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.
Correct.  BUt as you say, it might be more risky in other ways.

 > But perhaps this is the only specific case at the moment that is broken.
Hard to know.

 > I think we would want Alan's patch if/when we eliminate (subreg (mem))
 > though.
Maybe -- if we get rid of (subreg (mem)), we would fix nonzero_bits and
num_sign_bit_copies at the same time.    I poked briefly at this and I'm
pretty sure fixing nonzero_bits would avoid the problem without the need
for Alan's patch.

So, the question becomes, which patch do we want to go with?  I'll go with
the majority on this one since there's no clear right/wrong answer.

jeff