From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16530 invoked by alias); 21 Mar 2011 19:27:39 -0000 Received: (qmail 16520 invoked by uid 22791); 21 Mar 2011 19:27:37 -0000 X-SWARE-Spam-Status: No, hits=-6.1 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_XT,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 21 Mar 2011 19:27:26 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p2LJROQm011132 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 21 Mar 2011 15:27:24 -0400 Received: from anchor.twiddle.home (ovpn-113-104.phx2.redhat.com [10.3.113.104]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p2LJRN27018122; Mon, 21 Mar 2011 15:27:23 -0400 Message-ID: <4D87A69B.90704@redhat.com> Date: Mon, 21 Mar 2011 19:27:00 -0000 From: Richard Henderson User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.15) Gecko/20110307 Fedora/3.1.9-0.39.b3pre.fc14 Thunderbird/3.1.9 MIME-Version: 1.0 To: gcc-patches@gcc.gnu.org, patches@linaro.org, rdsandiford@googlemail.com Subject: Re: Cleaning up expand optabs code References: <4D825EF5.1010307@redhat.com> <87wrju28hn.fsf@firetop.home> In-Reply-To: <87wrju28hn.fsf@firetop.home> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2011-03/txt/msg01337.txt.bz2 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" > [(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~