This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Cleaning up expand optabs code
On 03/19/2011 12:52 PM, Richard Sandiford wrote:
> Given the mode stuff above, I've tried to be quite draconian as far
> as caller-provided modes go. I think the caller really should know
> what mode they're dealing with. The one case where I had to hold
> back a bit was create_convert_operand_from, which replaces things like:
>
> if (GET_MODE (op0) != mode0 && mode0 != VOIDmode)
> xop0 = convert_modes (mode0,
> GET_MODE (op0) != VOIDmode
> ? GET_MODE (op0)
> : mode,
> xop0, unsignedp);
>
> It seems a little suspicious that we only trust "mode" for CONST_INT
> op0s, and not for other cases, but I'd like to leave that for now.
> Maybe a future "clean up"?
Sure.
> Also, I had to change the i386 setmem pattern in order to avoid
> a regression in cold-attribute-1.c. The current predicate for
> the character operand is "const_int_operand", but the pattern
> handles registers as well. I'm sure there are going to be other
> things like that, so sorry in advance if this patch goes in and
> breaks a target...
Sure.
> * reload.c (find_reloads_address_1): Use insn_operand_matches.
> * reload1.c (gen_reload): Likewise.
All the bits that just use insn_operand_matches are approved.
You can commit those first if you like to reduce the patch size.
> {
> - if ((! (*insn_data[(int) CODE_FOR_prefetch].operand[0].predicate)
> - (op0,
> - insn_data[(int) CODE_FOR_prefetch].operand[0].mode))
> - || (GET_MODE (op0) != Pmode))
> - {
> - op0 = convert_memory_address (Pmode, op0);
> - op0 = force_reg (Pmode, op0);
> - }
> - emit_insn (gen_prefetch (op0, op1, op2));
> + struct expand_operand ops[3];
> +
> + create_address_operand (&ops[0], op0);
> + create_integer_operand (&ops[1], INTVAL (op1));
> + create_integer_operand (&ops[2], INTVAL (op2));
> + if (maybe_expand_insn (CODE_FOR_prefetch, 3, ops))
> + return;
> }
Yep, this interface is a definite cleanup.
> @@ -2452,10 +2443,11 @@ expand_builtin_interclass_mathfn (tree e
> if (mode != GET_MODE (op0))
> op0 = convert_to_mode (mode, op0, 0);
>
> - /* Compute into TARGET.
> - Set TARGET to wherever the result comes back. */
> - if (maybe_emit_unop_insn (icode, target, op0, UNKNOWN))
> - return target;
> + create_output_operand (&ops[0], target, TYPE_MODE (TREE_TYPE (exp)));
> + if (maybe_legitimize_operands (icode, 0, 1, ops)
> + && maybe_emit_unop_insn (icode, ops[0].value, op0, UNKNOWN))
> + return ops[0].value;
What are you doing here that maybe_emit_unop_insn doesn't?
> + if (maybe_expand_insn (unsignedp ? CODE_FOR_extzv : CODE_FOR_extv,
> + 4, ops))
> {
> - emit_insn (pat);
> + xtarget = ops[0].value;
> if (xtarget == xspec_target)
> return xtarget;
> - if (xtarget == xspec_target_subreg)
> + if (ops[0].value == xspec_target_subreg)
> return xspec_target;
Why this last change?
> x = prepare_operand (icode, x, 2, mode, compare_mode, unsignedp);
> y = prepare_operand (icode, y, 3, mode, compare_mode, unsignedp);
> comparison = gen_rtx_fmt_ee (code, result_mode, x, y);
> - if (!x || !y
> - || !insn_data[icode].operand[2].predicate
> - (x, insn_data[icode].operand[2].mode)
> - || !insn_data[icode].operand[3].predicate
> - (y, insn_data[icode].operand[3].mode)
> - || !insn_data[icode].operand[1].predicate (comparison, VOIDmode))
> + if (!x || !y)
> {
> delete_insns_since (last);
> return NULL_RTX;
Seems like we ought to push the IF above generating COMPARISON now.
> expand_widen_pattern_expr (sepops ops, rtx op0, rtx op1, rtx wide_op,
> rtx target, int unsignedp)
Geezam. I hope this one's correct -- the original code is impossible to
follow. I suspect that it's trying to handle so many different cases at
once that it can't really validate anything at all.
> + op = 0;
> + create_output_operand (&eops[op++], target, TYPE_MODE (ops->type));
> + create_convert_operand_from (&eops[op++], op0, tmode0, unsignedp);
> if (op1)
> + create_convert_operand_from (&eops[op++], op1, tmode1, unsignedp);
> if (wide_op)
> + create_convert_operand_from (&eops[op++], wide_op, wmode, unsignedp);
> + expand_insn (icode, op, eops);
> + return eops[0].value;
... the conversion is at least legible.
> + if (commutative_p)
> + make_operand_commutative (&ops[1], 0);
So this is used exactly once here in expand_binop_directly?
Honestly, I found the description of make_operand_commutative to
be rather weak, and the implementation,
> + for (i = 0; i + 1 < nops; i++)
> + if (ops[i].commutative < MAX_EXPAND_OPERANDS
> + && swap_commutative_operands_with_target (ops[ops[i].commutative].value,
> + ops[i].value,
> + ops[i + 1].value))
with the assumption of i & i+1 being related, to be a pretty strong
assumption.
I think perhaps we should omit commutative operands as a feature of the
new interface -- at least until we have more than one user and can firm
up the semantics. Instead you can simply use maybe_legitimize_operands
here directly, and do the commutative thing right here inline.
> + case EXPAND_INTEGER:
> + mode = insn_data[(int) icode].operand[opno].mode;
> + if (mode != VOIDmode
> + && (trunc_int_for_mode (INTVAL (op->value), mode)
> + == INTVAL (op->value)))
> + goto input;
> + break;
Surely const_int_operand (op->value, mode).
> Index: gcc/config/i386/i386.md
> ===================================================================
> --- gcc/config/i386/i386.md 2011-03-19 17:12:02.000000000 +0000
> +++ gcc/config/i386/i386.md 2011-03-19 17:12:15.000000000 +0000
> @@ -15793,7 +15793,7 @@ (define_insn "*rep_movqi"
> (define_expand "setmem<mode>"
> [(use (match_operand:BLK 0 "memory_operand" ""))
> (use (match_operand:SWI48 1 "nonmemory_operand" ""))
> - (use (match_operand 2 "const_int_operand" ""))
> + (use (match_operand 2 "nonmemory_operand" ""))
I do wonder if this operand ought to have QImode. We do have
> if (valmode != QImode)
> val = gen_lowpart (QImode, val);
inside promote_duplicated_reg, but it does seem like leaving
the mode unspecified in the md file is a mistake.
r~