PR37363: PR36090 and PR36182 all over again

[Richard Sandiford]

Paolo Bonzini <bonzini@gnu.org> writes:
>>> In case of cris, the predicate goes into general_operand, which does
>>>
>>>   if (CONSTANT_P (op))
>>>     return ((GET_MODE (op) == VOIDmode || GET_MODE (op) == mode
>>>              || mode == VOIDmode)
>>>             && (! flag_pic || LEGITIMATE_PIC_OPERAND_P (op))
>>>             && LEGITIMATE_CONSTANT_P (op));
>>>
>>> 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:
>
> (define_expand "movsi"
>   [(set
>     (match_operand:SI 0 "nonimmediate_operand" "")
>     (match_operand:SI 1 "cris_general_operand_or_symbol" ""))]
>   ...
>
> (define_special_predicate "cris_general_operand_or_symbol"
>   (ior (match_operand 0 "general_operand")
>        (and (match_code "const, symbol_ref, label_ref")
>             ...

As H-P says, the predicates on move expanders are generally ignored.
emit_move_insn & subroutines deliberately don't check them.
(Our usual method of making things match predicates is to force them
into a register, but that could easily lead to infinite recursion if
applied to moves.)

FWIW, in MIPS, I have a port policy of not adding predicates
to move expanders.  I think adding them just creates confusion,
because readers will think they actually mean something.

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

As you say, force_operand currently does nothing with constants.
My understanding is that that really is by design (in the loosest
possible sense of the word).  As H-P says, it's then the move expander's
responsibility to handle the thing.

I know it's not the cleanest interface, but many ports are written
to follow this behaviour.

TBH, as I said in comment #11 in PR36182, I think rs6000 is being
overly cute with the representation of a TOC entry as:

   (const (minus foo (symbol_ref "*.LCTOC1"))).

It might be accurate, but (symbol_ref "*.LCTOC1") has no real meaning
to GCC.  And as we can see from these bugs, the extra symbol_ref just
tends to get in the way.

Other ports would generally use an unspec here.  (E.g. most ports
use an unspec for GOT-relative constants, even though:

   (const (minus foo (symbol_ref "_GLOBAL_OFFSET_TABLE_")))

would in some cases be accurate.)  I think using an unspec in rs6000
would solve some of the port-specific issues.  In particular, I don't
think 36090 would have happened with an unspec representation.

Richard