[RFA:] Adjust documentation for LEGITIMATE_CONSTANT_P et al to match reality

[Richard Sandiford]

Hans-Peter Nilsson <hans-peter.nilsson@axis.com> writes:
>> From: Richard Sandiford <rdsandiford@googlemail.com>
>> Date: Sun, 07 Sep 2008 20:45:38 +0100
>
>> Hans-Peter Nilsson <hans-peter.nilsson@axis.com> writes:
>> Well, my message said both what I think the current rules are
>> (first set of grammar rules) and what I think they ought to be
>> (second set of grammar rules).  As for implementing it, that's
>> what I started trying to do with my PowerPC patch.
>> 
>> > In the meantime, let's have documentation that matches the code.
>> 
>> But my point was that the proposed documentation doesn't match the code.
>
> Well, I think it does.  No rules are explicit there.
> If it's overly general, that's erroring on the safe side.

Sure, no rules are explicit in the form of a comment, but this
is really a documentation deficiency too.  (We're agreed that
the documentation needs to be better. ;))

Let me put it this way: the documentation used to match the code.
Setting LEGITIMATE_CONSTANT_P to 1 used to be a perfectly reasonable
thing to do.  arc, cris, crx, fr30, iq2000, m68h11, mn10300, picochip,
score, stormy16 and xtensa still do that.  (OK, not exactly a roll-call
of thriving targets, bar some notable exceptions.  But still, these
ports did work once.)  Unfortunately, the documentation didn't say _why_
this was OK, but I still claim it is because of the unwritten rules I
gave earlier.  Writing down those rules would be a good thing.

We have now have code that makes it dangerous to set LEGITIMATE_CONSTANT_P
to 1, and your patch is trying to update the documentation to match that code.
But:

   (a) There are still unwritten rules, especially when it comes to
       functions like force_reg.  Claiming otherwise puts every port
       in the sin bin.  See below for more on that.

   (b) I think we're being far too quick to dismiss the idea that
       new code is buggy.  It goes against the documentation and a
       long-standing tradition.  And it likely affects many of the
       ports listed above, not just cris.

So which contra-documentation code changes are we talking about?
The only example I've seen is the simplification code, particularly
simplify_plus_minus.

> If we *do* define rules, then the target still has to validize
> the subset it accepts, so saying "you can get any arithmetic
> canonical RTX in a CONST, please validize what you handle by
> defining these macros" isn't that far from "you can get any of
> these:... in a CONST, please validize what you handle by
> defining these macros".

But there isn't a validisation macro for every case.  In particular,
there's no macro to say whether a CONST can be passed to a move
expander.  In the absence of such a macro, and in the absence of a more
detailed description of which CONSTs are valid when, the documentation
could be read as saying that any CONST can be passed to a move expander.
That's how I read your patch originally, hence the confusion.

It seems you weren't trying to say that after all, which is good.
But in that case, I think the patch should say what kinds of CONST
a move expander _does_ have to handle, or least exempt that case
from "CONST can contain anything".  Otherwise it gives a false impression.

>> You seem to be claiming that a move expander has to cope with a CONST
>> containing any unsimplifiable rtx expression whose terms are CONST_OBJs
>> or UNSPECs (UNSPECs should be treated as black-box terms here),
>> or something akin to that.
>
> No, I certainly didn't.  Attempted transformations to the RTX
> are validized, so all the target has to do is provide the
> "right" predicate macros.  Move-insns are only created for
> whatever CONST contents were accepted originally.  Maybe there
> are rules as to what trees are transformed into RTX, so we don't
> get the ashiftrt kind of oddity in the first place.

Right, exactly.  But, going back to the above, those rules aren't
AFAIK written down in the code anywhere either.  If we're prepared
to accept that the rules exist anyway, why not do the same for
simplification?

>> >> E.g. most ports would barf at:
>> >> 
>> >>    (const (ashiftrt (and (symbol_ref ...)
>> >>                          (const_int ...))
>> >>                     (const_int ...)))
>> >> 
>> >> Does that count as "algorithmically valid"?
>> >
>> > If there are such operators with data dependence on
>> > symbol_ref's, I don't see why stops that from happening.
>> 
>> but it's clear that no port currently handles this, even though
>> this expression can be constructed in C code using intptr_ts.
>
> Again, all that is needed is that it's refused by the macros I
> touched in the documentation update.

Yes, but many ports don't do that.  Those parts match the current
documentation, and they used to work.  And many of the targets that
define LEGITIMATE_CONSTANT_P to zero in some cases do so for expressions
that we've always had, such as certain const_doubles, or certain
(const (plus symbol_ref const_int))s.

So in terms of documenting what the code does, we have a contradiction.
It's then a case of: which code do you believe?

>> >> Likewise, the target-independent code must have some assurances about
>> >> the kinds of CONST that the target code creates.  Otherwise, it won't
>> >> be able to make any guarantees about the expressions it creates itself.
>> >> E.g. if a backend creates something that simplifies to (const (plus X X)),
>> >> can/should the target-independent code rewrite that as
>> >> (const (ashift X 2))?
>> >
>> > A target would be right to refuse non-canonical RTX and for
>> > that, the canonical representation is (MULT X 2).
>> 
>> It depends on context.  ASHIFT is canonical in some places, MULT in others.
>
> Uh, that's a pre-existing flaw, you can't expect me to fix that
> too.

I wasn't trying to.  I was just saying that the original question
(s/2/1/ ;)) wasn't necessarily as wrong-headed as you claimed.

Richard