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:

> +; 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.  ]

> +(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.

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

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

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com