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