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:
> But the single most important take home message from my posting above is
> summarized by it's title "LOAD_EXTEND_OP only applies to (subreg (mem
> ...))".  Basically, the only time that this target macro has any
> meaning within the middle and back ends, is for dealing with paradoxical
> subregs of MEMs.

It seems the point of that message,
<URL:http://gcc.gnu.org/ml/gcc-patches/2002-07/msg00383.html> is
to mean "as opposed to subreg of *REG*" not "only applies to
subreg".

I don't *really* agree with the subreg part: just the MEM part.
The *subreg* part is not reflected in the documentation of
LOAD_EXTEND_OP nor all of the code, neither do I think it should
be.

> You're therefore in a difficult position of arguing that LOAD_EXTEND_OP
> is the best way of representing MMIX's load/extension semantics whilst
> at the same time arguing that paradoxical subregs of MEMs should be
> eliminated.  The fate of both is irrevocably intertwined.

I beg to disagree!  LOAD_EXTEND_OP (as a description of target
semantics) does not or at least should not automatically depend
on the presence of subreg; only of MEM.  (If it eventually is
only used for subregs, that's a bug.)  See cse_insn,
fold_single_bit_test, fold, reload_cse_simplify_set,
reload_cse_simplify_operands, nonzero_bits1,
num_sign_bit_copies1 (the MEM case), which aren't trigged by the
presence of a subreg rtl (at least not directly).

> Unless you have some thought's on eliminating paradoxical SUBREGs whilst
> preserving LOAD_EXTEND_OP?  :>

Yes I do, as I mentioned in passing.  Actually I think not all
of the current use is with subregs (not really as intertwined as
you describe it).  Besides that, as I mentioned in passing, GCC
could generate sign/zero_extend and recog could match
(zero/sign_extend (mem ...)) against target description (mem
...) and be adjusted after all RTL optimizers.  It may be
deferred to the target to adjust for intermediate existence,
like when target macros are used.  Maybe something like the
present MEM_P would help (please don't take this verbatim):
#ifdef LOAD_EXTEND_OP
#define MEM_P(X) \
 (GET_CODE (X) == MEM \
  || ((GET_CODE (X) == SIGN_EXTEND || GET_CODE (X) == ZERO_EXTEND)
      && LOAD_EXTEND_OP (GET_MODE (XEXP (X, 0))) == GET_CODE (X)))
#else
#define MEM_P(X) (GET_CODE (X) == MEM)
#endif

>  Another smiley, paradoxical subregs are
> too silly to be taken too seriously.  8-)

Does that mean you agree paradoxical mem subregs should go away?

> On Wed, 29 Dec 2004, Hans-Peter Nilsson wrote:
> > LOAD_EXTEND_OP-only-targets are not helped by those paradoxical
> > mem subregs.  Those subregs should never be.
>
> I believe this is where we disagree.

I assume you refer to the combination, not the "those subregs
should never be" part?  (Modulo bugs, maybe the subregs happen
to help *at present*, but the bugs and ambiguity is
overwhelming, and they should go.  They definitely don't help
when it comes to maintenance.)

> ...
> Ok then, keep with your original description.  I just wanted to
> keep it clear that we're not "globally" disabling the creation
> of paradoxical SUBREGs, merely preventing an unsafe/inappropriate
> transformations on them.

Right and *only* for calls to that function, but that's implied
from the function-context part of the ChangeLog.

> Why not the address-calculation-for-stripped-subreg
> correction (at least "in addition")?

> I've not given enough thought to your first patch to either approve
> or disapprove of it, yet.  Indeed, the reason I asked why the second
> patch is sufficient was solely to avoid thinking about this issue.

Ok, I suppose I was unclear (hmm, I see, but I also wrote "for
each patch individually: built cross to mmix-knuth-mmixware,
fixing all test-cases in PR target/18701, no regressions").
I'll try to remember to adjust for turkey at this time of the
year. ;-)

> >From you description, its emphasis and examples, it wasn't immediately
> obvious that the first transformation was incorrect.  As implied above
> we clearly  have different opinions on the pros/cons of paradoxical
> subregs.  However, I immediately agreed that the transformation that
> was addressed by your second patch was clearly wrong.  Given that we
> both agree that fixing this is the right thing to do, I thought I'd
> approve it.

Thanks.  It was a bit unclear to me whether it was approved or
you were just asking for more information.

> If you're really going to force me to think about SUBREG offsets on
> different endian architectures, that arbitrarily sign extend or zero
> extend or don't extend, and its nebulous INSN_SCHEDULING interactions,
> it'll just take a while longer (especially after this much turkey).

:-)

> Let me give your first patch the attention it deserves.

Thanks (and thanks for the review)!

brgds, H-P