> Date: Sat, 06 Sep 2008 12:50:18 +0200 > From: Paolo Bonzini <[EMAIL PROTECTED]>
> 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
