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