Andreas Krebbel wrote: > On 02/01/2016 02:30 PM, Ulrich Weigand wrote: > > This seems to add just a single alternative. Is that OK if it gets > > substituted into a pattern that used multiple alternatives otherwise? > Yes. This is supposed to work. The new constraint will be duplicated until > it matches the number > of alternatives in the original insn.
OK, that makes sense. But now I'm wondering about this bit of the ashiftrt patch: +; FIXME: The number of alternatives is doubled here to match the fix +; number of 4 in the subst pattern for the (clobber (match_scratch... +; The right fix should be to support match_scratch in the output +; pattern of a define_subst. +(define_subst "cconly_subst" + [(set (match_operand:DSI 0 "" "") + (match_operand:DSI 1 "" "")) + (clobber (reg:CC CC_REGNUM))] + "s390_match_ccmode(insn, CCSmode)" + [(set (reg CC_REGNUM) + (compare (match_dup 1) (const_int 0))) + (clobber (match_scratch:DSI 0 "=d,d,d,d"))]) I guess this is where the number 4 comes from? But if the alternatives are duplicated, shouldn't it then work to simply use: (clobber (match_scratch:DSI 0 "=d"))]) > >> +; In the subst pattern we have to disable the alternative where op2 is > >> +; already a constant. This attribute is supposed to be used in the > >> +; "disabled" insn attribute to achieve this. > >> +(define_subst_attr "addr_style_op_disable" "addr_style_op_subst" "0" "1") > > > > Is this necessary given that the subst adds a REG_P (operands[2]) > > condition? > I wasn't sure about this. How does reload/lra behave when the constraints > allow more than the insn > condition? I expected that reload might remember that one of the alternatives > might take a constant > as well and then tries as part of reload inheritance and such things to > actually exploit this?! Just > disabling the alternative in order to prevent reload from recording stuff for > that alternative > appeared safer to me. > > On the other hand it is not ideal to have an operands[x] check in the insn > condition in the first > place. Especially when using define_substs where the operands get renumbered > this might add hard to > find bugs. Perhaps I should try replacing the insn predicate with a > subst_attr instead? Is having a > constraint which allows more than a predicate any better than having the same > with the insn condition? Hmm. In theory reload should always respect both constraints and predicates. But I guess it can't hurt to disable the alternative just to be safe; it is indeed an uncommon case to have the constraint accept more than the predicate. (Also a PLUS with two CONST_INT operands is not canonical RTL anyway.) Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain ulrich.weig...@de.ibm.com
