Re: PR37363: PR36090 and PR36182 all over again

[Paolo Bonzini <bonzini at gnu dot org>]

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

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

>>   (*) 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.

> Signalling that they are not legitimate means they can still be
> handled by a move.

That's why I used the predicate.

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

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

> (FWIW, I'll add a LEGITIMATE_CONSTANT_P to CRIS just to cover my
> bases.  It won't solve the basic problem, because that could
> just cause that invalid CONST contents in PR37363 and PR36182 to
> end up in a move insn instead.)

I don't think so, because general_operand would pass the CONST to your
LEGITIMATE_CONSTANT_P, and hence cause it to be rejected.

Paolo