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

[Richard Henderson <rth at redhat dot com>]

On 08/13/2012 10:07 AM, Ulrich Weigand wrote:
>> +/* 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.)

Absolutely correct; I hadn't considered that.

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

Yes.

>> -(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?

I blatantly assumed that L+RISBGZ was better than ICM+SRL.  I probably
shouldn't have also disabled the sign_extract version as well.

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

Ok.

>> +   && 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.)

Good catch.


r~