[PATCH 0/7] s390 improvements with r[ioxn]sbg

[Ulrich Weigand]

Richard Henderson wrote:
> Only "tested" visually, by examining assembly diffs of the
> runtime libraries between successive patches.  All told it
> would appear to be some remarkable code size improvements.

Thanks for having a look at this!

> Please test.

Unfortunately GCC crashes during bootstrap, probably because on
of the issues below.

A couple of comments to the patches:

>   s390: Constraints, predicates, and op letters for contiguous bitmasks
>   s390: Only use lhs zero_extract in word_mode
>   s390: Use risbgz for AND.
>   s390: Add mode attribute for mode bitsize

These look all good to me.

>   s390: Implement extzv for z10

> +/* Check whether a rotate of ROTL followed by an AND of CONTIG is equivalent
> +   to a shift followed by the AND.  In particular, CONTIG should not overlap
> +   the (rotated) bit 0/bit 63 gap.  */
> +
> +bool
> +s390_extzv_shift_ok (int bitsize, int rotl, unsigned HOST_WIDE_INT contig)
> +{
> +  int pos, len;
> +  bool ok;
> +
> +  ok = s390_contiguous_bitmask_p (contig, bitsize, &pos, &len);
> +  gcc_assert (ok);
> +
> +  return (rotl <= pos || rotl >= pos + len + (64 - bitsize));
> +}

I don't quite see how this can work correctly for both left and right shifts.
E.g. for bitsize == 64, rotl == 1, contig == 1, a left shift by 1 followed
by the AND is always zero, and certainly not equal to the rotate.  But the
routine would return true.  (Note that the same routine would be called for
a *right* shift by 63, in which case the "true" result is actually correct.)

> +(define_insn "extzv"
> +  [(set (match_operand:DI 0 "register_operand" "=d")
> +       (zero_extract:DI
> +         (match_operand:DI 1 "register_operand" "d")
> +         (match_operand 2 "const_int_operand" "")
> +         (match_operand 3 "const_int_operand" "")))
> +   (clobber (reg:CC CC_REGNUM))]
> +  "TARGET_Z10"
> +  "risbg\t%0,%1,63-%3-%2,128+63,63-%3-%2"

This doesn't look right for bits-big-endian order.  Shouldn't we have a
rotate count of %3+%2, and a start bit position of 64-%2 ?

> -(define_insn_and_split "*extv<mode>"
> +(define_insn_and_split "*pre_z10_extv<mode>"
>    [(set (match_operand:GPR 0 "register_operand" "=d")
>         (sign_extract:GPR (match_operand:QI 1 "s_operand" "QS")
> -                         (match_operand 2 "const_int_operand" "n")
> +                         (match_operand 2 "nonzero_shift_count_operand" "")
>                           (const_int 0)))
>     (clobber (reg:CC CC_REGNUM))]
> -  "INTVAL (operands[2]) > 0
> -   && INTVAL (operands[2]) <= GET_MODE_BITSIZE (SImode)"
> +  "!TARGET_Z10"
>    "#"
>    "&& reload_completed"

Why disable this for pre-z10?

> +(define_insn "*extzv_<mode>_srl"
> +  [(set (match_operand:DSI 0 "register_operand" "=d")
> +       (and:DSI (lshiftrt:DSI
> +                  (match_operand:DSI 1 "register_operand" "d")
> +                  (match_operand:DSI 2 "nonzero_shift_count_operand" ""))
> +               (match_operand:DSI 3 "contiguous_bitmask_operand" "")))
> +   (clobber (reg:CC CC_REGNUM))]
> +  "TARGET_Z10

While those are equivalent (given that TARGET_Z10 implies TARGET_ZARCH),
it would seem more in line with many other patterns to use GPR instead of DSI.

>   s390: Generate rxsbg, and shifted forms of rosbg

This looks OK.

>   s390: Generate rnsbg

> +(define_insn "*insv_rnsbg_srl"
> +  [(set (zero_extract:DI
> +         (match_operand:DI 0 "nonimmediate_operand" "+d")
> +         (match_operand 1 "const_int_operand" "")
> +         (match_operand 2 "const_int_operand" ""))
> +       (and:DI
> +         (lshiftrt:DI
> +           (match_dup 0)
> +           (match_operand 3 "const_int_operand" ""))
> +         (match_operand:DI 4 "nonimmediate_operand" "d")))
> +   (clobber (reg:CC CC_REGNUM))]
> +  "TARGET_Z10
> +   && INTVAL (operands[3]) == 64 - INTVAL (operands[1]) - INTVAL (operands[2])"
> +  "rnsbg\t%0,%4,%2,%2+%1-1,64-%2,%1"

I guess the last "," is supposed to be a "-".  (Then we might
as well use %3 instead of 64-%2-%1.)

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com