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