Re: [PATCH 3/9] S/390: Get rid of Y constraint in rotate patterns.

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

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.Weigand@de.ibm.com