PR37363: PR36090 and PR36182 all over again

Hans-Peter Nilsson hans-peter.nilsson@axis.com
Sat Sep 6 12:27:00 GMT 2008


> Date: Sat, 06 Sep 2008 12:50:18 +0200
> From: Paolo Bonzini <bonzini@gnu.org>

> Still, having a new target hook for this seems overkill.

But it's better than abusing an old macro with a slightly
different use.

>  For example,
> since ports do have to deal with complicated constants when they expand
> moves, and since some of them already look inside CONSTs in their
> LEGITIMATE_CONSTANT_P, another possibility to throw in the air is
> something like (better names welcome...)
> 
> rtx
> avoid_terrible_constants (rtx x)

valid_const_contents (name adjusted for use as per below).

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

Did you mean this as a short-term or long-term solution?
(Mind, we already have a proposed short-term 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.

Signalling that they are not legitimate means they can still be
handled by a move.  Not only the documentation, but, again,
you'd have to replace all its uses, because current users just
call force_reg or force_const_mem for a negative!  And then
perhaps you'd have to invent a new macro for the old documented
behavior (...which might be just as good, considering the
confusing name).

> I'm absolutely unsure that this is the way to go; but it has two
> advantages: 1) not leaking really bad constants outside simplify-rtx.c;

We can do that.

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

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.

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

Let's just have the new target hook and use it in *this* place
(valid_const_contents) for a starter.  For now, the default
always returning "yup, that CONST-candidate you're trying to
form is valid".  I can write a candidate to use with CRIS but
usable for mainstream ELF targets to use (though it'd have to
refuse sym2-sym1 expressions where sym1 is in the current
section).

brgds, H-P



More information about the Gcc mailing list