Re: PR37363: PR36090 and PR36182 all over again

[Hans-Peter Nilsson <hans-peter dot nilsson at axis dot com>]

> Date: Sat, 06 Sep 2008 15:14:46 +0200
> From: Paolo Bonzini <bonzini@gnu.org>

> >> H-P can check for the problematic case inside his LEGITIMATE_CONSTANT_P
> >> (*), or add a move expander for it.
> > 
> > I think you're mixing up CRIS and rs6000, the latter which
> > generated something it had to handle but which was munged,
> > PR36090.  CRIS is mainstream in that sense.  (You'd have to get
> > buy-in from David Edelsohn on a LEGITIMATE_CONSTANT_P definition
> > in rs6000 if PR36090 resurfaces.)
> 
> This is from CRIS:

Oh, I misunderstood you.  By "problematic case" I thought you
meant the invalid or nonstandard CONSTs mentioned in the PRs!

> > Did you mean this as a short-term or long-term solution?
> > (Mind, we already have a proposed short-term solution.)
> 
> As a long term solution.  Though not in that exact shape -- I wanted to
> have discussion on it and converge together to a real solution.

Very well.

> >>   (*) but then does this mean the documentation for L_C_P is obsolete,
> >>   and returning 1 is not necessarily a good thing to do for targets with
> >>   sections?  Maybe there is a better definition that can be the default?
> > 
> > Again, LEGITIMATE_CONSTANT_P is the wrong macro, it's for
> > checking constants which are appropriate as immediate operands
> > (to non-move insns), not for being at-all-legitimate.
> 
> LEGITIMATE_CONSTANT_P is just what is used by general_operand.  I'm
> proposing another use of *the predicate for mov's operand 1*, not of
> LEGITIMATE_CONSTANT_P.  With the above questions, I was expressing my
> doubts on the doc for LEGITIMATE_CONSTANT_P in general.

Definitely.  Not sure that LEGITIMATE_CONSTANT_P is used
consistently too.

The doc's are based on the assumption that whatever is in the
CONST is benevolent and sane, either created by the target or
e.g.  symbol_ref + const_int.  The simplify-rtx optimization was
dreaming up a new one, (minus sym2 sym1).  The sad thing is that
*this* point (the CONST creation, simplify_plus_minus) isn't
really where it should be sanitized, because the constant could
very much just be a algorithmically correct "dream", intended to
just be put in a REG_EQUAL note.  (Which is exactly what happens
*first*; *then* a few lines, src of the insn is replaced too.)

Or, maybe the constant is not fit for any operand but as a
constant in memory!  *Then* we'd need a hook to sanitize
that...but maybe let's blow up that bridge only if we get to it.

A helper function, e.g. "trivial_constant_expression_p" that
returns true only if there's just a non-CONST CONST_OBJ or an
(const (plus (symbol_ref foo) (const_int N))) would do as the new
default for LEGITIMATE_CONSTANT_P.

> > Signalling that they are not legitimate means they can still be
> > handled by a move.
> 
> That's why I used the predicate.

But if the insn where the CONST is to be used isn't a move,
that's the wrong test...hm.

Ok, I stepped through the failing test, and the change in the
insn *is* validized, so a checking LEGITIMATE_CONSTANT_P would
help there.

> >> 2) it makes clear how to fix bugs -- you restrict
> >> LEGITIMATE_CONSTANT_P/LEGITIMATE_PIC_OPERAND_P or add a move expander.
> > 
> > Contradicting current use, where anything that's found in a
> > non-LEGITIMATE_CONSTANT_P/LEGITIMATE_PIC_OPERAND_P must be
> > handled by a move expander!
> 
> Not necessarily; anything that's found in a non-legitimate constant must
> be handled by force_reg, and force_reg also tries using force_operand if
> what it gets is not a general_operand.  But maybe it's necessary to add a
> 
>   if (GET_CODE (value) == CONST)
>     value = XEXP (value, 0);
> 
> in force_operand.

Um, yeah, that might work (as amended by the two follow-up
messages).

> > To wit: a new bug would surface: you could here form something
> > that wasn't LEGITIMATE_CONSTANT_P but which was handled by a
> > move expander, and you'd force this into an insn which *isn't* a
> > move.  N.B. the insn in PR36182 wasn't a move.
> 
> Shouldn't the insn fail recognization, then?

Only with a LEGITIMATE_CONSTANT_P catching it...

So, can we agree on some or all of:

1. This (PR37363/PR36182) and PR36090 (in both ports) and
   whatever other port will be affected should be solved by a
   stricter LEGITIMATE_CONSTANT_P check, and where
   canonicalization is undefined (and a new definition can't get
   consensus agreed upon), the port has to check itself for
   whatever RTL expression it accepts.

2. Change the LEGITIMATE_CONSTANT_P documentation.

3. Change the default of LEGITIMATE_CONSTANT_P to a helper
   function, maybe trivial_constant_expression_p above.

brgds, H-P