On 02/01/2016 05:58 PM, Ulrich Weigand wrote: > 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"))])
As the comment tries (and fails) to say the duplication of constraints currently does not seem to work for match_scratch - only for match_operand. This is just an ugly way to work around this for now. The real fix is to enable match_scratch as well. >>>> +; 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.) Does this mean you would prefer to convert the insn condition into a predicate and change it with subst_attr or should I leave it in the insn condition? -Andreas- > > Bye, > Ulrich >
