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

[Andreas Krebbel <krebbel at linux dot vnet dot ibm dot com>]

On 02/01/2016 02:30 PM, Ulrich Weigand wrote:
> Andreas Krebbel wrote:
> 
>> +; Accept single register and immediate operands usable as shift
>> +; counts.
>> +(define_predicate "addrreg_or_constint_operand"
>> +  (match_code "reg, subreg, const_int")
> 
> I'm wondering whether this is even necessary.
> 
>> +{
>> +  if (GET_MODE (op) != VOIDmode
>> +      && GET_MODE_CLASS (GET_MODE (op)) != MODE_INT)
>> +    return false;
> 
> The predicate always seems to be used with a mode, so any extra
> mode checks here seem redundant.
> 
>> +  while (op && GET_CODE (op) == SUBREG)
>> +    op = SUBREG_REG (op);
>> +
>> +  if (REG_P (op)
>> +      && (REGNO (op) >= FIRST_PSEUDO_REGISTER || ADDR_REG_P (op)))
>> +    return true;
>> +
>> +  if (CONST_INT_P (op))
>> +    return true;
> 
> This looks somewhat copied from shift_count_or_setmem_operand, where
> we have a comment:
>   /* Don't allow any non-base hard registers.  Doing so without
>      confusing reload and/or regrename would be tricky, and doesn't
>      buy us much anyway.  */
> 
> With the new set up, I don't believe there is any issue with confusing
> reload -- it's no more complex than any other pattern now.  So it
> should be fine to just accept any register.
> 
> In fact, I'm starting to wonder whether it wouldn't be just fine to
> simply using nonmemory_operand instead of this special predicate
> (the only issue might be that we could get CONST_WIDE_INT, but it
> should be simple to handle those).
> 
>> +(define_expand "rotl<mode>3"
>> +  [(set (match_operand:GPR 0 "register_operand" "")
>> +        (rotate:GPR (match_operand:GPR 1 "register_operand" "")
>> +		    (match_operand:SI 2 "shift_count_or_setmem_operand" "")))]
> 
> Similarly, I think the expander no longer needs to use the special predicate
> shift_count_or_setmem_operandm but could just use nonmemory_operand.
> [ This will reject the PLUS that shift_count_or_setmem_operand accepted,
> but I don't believe you ever *could* get the PLUS in the expander anyway.
> It will only be moved in by combine, so as long as the insn accepts it,
> everything should be good.  ]
Ok. I'll try to get rid of the special predicates.

>> +(define_insn "*rotl<mode>3<addr_style_op><masked_op>"
>> +  [(set (match_operand:GPR             0 "register_operand"           "=d,d")
>> +	(rotate:GPR (match_operand:GPR 1 "register_operand"            "d,d")
>> +		    (match_operand:SI  2 "addrreg_or_constint_operand" "a,n")))]
>> +  "TARGET_CPU_ZARCH"
>> +  "@
>> +   rll<g>\t%0,%1,<addr_style_op_op3>(%2)
>> +   rll<g>\t%0,%1,%Y2"
> 
> So it looks like %Y is now always used for just a constant.  Maybe the
> implementation of %Y should be adjusted (once the patch series is complete)?
> Also, if we use just nonmemory_operand, %Y should be updated to accept
> CONST_WIDE_INT just like e.g. %x does.
Ok.

> 
>> +; This expands an register/immediate operand to a register+immediate
>> +; operand to draw advantage of the address style operand format
>> +; providing a addition for free.
>> +(define_subst "addr_style_op_subst"
>> +  [(set (match_operand:DSI 0 "" "")
>> +        (SUBST:DSI (match_operand:DSI 1 "" "")
>> +		   (match_operand:SI 2 "" "")))]
>> +  "REG_P (operands[2])"
>> +  [(set (match_dup 0)
>> +        (SUBST:DSI (match_dup 1)
>> +		   (plus:SI (match_dup 2)
>> +			    (match_operand 3 "const_int_operand" "n"))))])
> 
> 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.

>> +; 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?

-Andreas-