From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2928 invoked by alias); 30 Mar 2011 08:53:43 -0000 Received: (qmail 2904 invoked by uid 22791); 30 Mar 2011 08:53:40 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-wy0-f175.google.com (HELO mail-wy0-f175.google.com) (74.125.82.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 30 Mar 2011 08:53:32 +0000 Received: by wye20 with SMTP id 20so1016170wye.20 for ; Wed, 30 Mar 2011 01:53:31 -0700 (PDT) Received: by 10.216.205.139 with SMTP id j11mr911602weo.28.1301475211342; Wed, 30 Mar 2011 01:53:31 -0700 (PDT) Received: from richards-thinkpad (gbibp9ph1--blueice2n1.emea.ibm.com [195.212.29.75]) by mx.google.com with ESMTPS id d54sm2341287wej.10.2011.03.30.01.53.28 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 30 Mar 2011 01:53:29 -0700 (PDT) From: Richard Sandiford To: Richard Henderson Mail-Followup-To: Richard Henderson ,Mikael Pettersson , Andreas Krebbel , gcc-patches@gcc.gnu.org, richard.sandiford@linaro.org Cc: Mikael Pettersson , Andreas Krebbel , gcc-patches@gcc.gnu.org Subject: Re: Cleaning up expand optabs code References: <4D825EF5.1010307@redhat.com> <87wrju28hn.fsf@firetop.home> <4D87A69B.90704@redhat.com> <4D88E10A.8080005@redhat.com> <4D8B32A9.9050309@linux.vnet.ibm.com> <19857.52792.228466.501327@pilspetsen.it.uu.se> <4D925671.5030402@redhat.com> Date: Wed, 30 Mar 2011 09:13:00 -0000 In-Reply-To: <4D925671.5030402@redhat.com> (Richard Henderson's message of "Tue, 29 Mar 2011 15:00:17 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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/msg02049.txt.bz2 Richard Henderson writes: > On 03/29/2011 06:21 AM, Richard Sandiford wrote: >> - enum machine_mode mode0 = insn_data[(int) icode].operand[1].mode; >> - enum machine_mode mode1 = insn_data[(int) icode].operand[2].mode; >> - enum machine_mode tmp_mode; >> + enum machine_mode xmode0 = insn_data[(int) icode].operand[1].mode; >> + enum machine_mode xmode1 = insn_data[(int) icode].operand[2].mode; >> + enum machine_mode mode0, mode1, tmp_mode; > ... >> - if (GET_MODE (xop0) != mode0 && mode0 != VOIDmode) >> - xop0 = convert_modes (mode0, >> - GET_MODE (xop0) != VOIDmode >> - ? GET_MODE (xop0) >> - : mode, >> - xop0, unsignedp); >> - >> - if (GET_MODE (xop1) != mode1 && mode1 != VOIDmode) >> - xop1 = convert_modes (mode1, >> - GET_MODE (xop1) != VOIDmode >> - ? GET_MODE (xop1) >> - : mode, >> - xop1, unsignedp); >> + mode0 = GET_MODE (xop0) != VOIDmode ? GET_MODE (xop0) : mode; >> + if (xmode0 != VOIDmode && xmode0 != mode0) >> + { >> + xop0 = convert_modes (xmode0, mode0, xop0, unsignedp); >> + mode0 = xmode0; >> + } >> + >> + mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : mode; >> + if (xmode1 != VOIDmode && xmode1 != mode1) >> + { >> + xop1 = convert_modes (xmode1, mode1, xop1, unsignedp); >> + mode1 = xmode1; >> + } > > This smells like a target bug, but I can't quite put my finger > on exactly what's wrong, and I haven't debugged the original. > > The backend has said xmode[01] is what it expects. The only > reason you wouldn't write xmode[01] in the create_input_operand > line is if xmode[01] are VOIDmode. Right. Sorry, I should have said, but the failure was actually from: case EXPAND_INPUT: input: gcc_assert (mode != VOIDmode); expand_binop_direct passed the match_operand's mode to create_input_operand, which was really the opposite of the intention. The caller should be passing the mode of the rtx, which it should always "know". > That seems like mistake number one, particularly for a binop, > but I'll accept that for the nonce. Which pattern triggered > the problem in the target? It was ashrdi3: (define_expand "ashrdi3" [(set (match_operand:DI 0 "register_operand" "") (ashiftrt:DI (match_operand:DI 1 "register_operand" "") (match_operand 2 "const_int_operand" "")))] "!TARGET_COLDFIRE" Which seems reasonable in this context, since the shift count isn't really DImode. > Then we've got the conditionalization in the init of mode[01], > which is presumably to handle CONST_INT as an input. > > What about something like > > xmode0 = insn_data... > if (xmode0 == VOIDmode) > xmode0 = mode; > > mode0 = GET_MODE (xop0); > if (mode0 == VOIDmode) > mode0 = mode; > > if (xmode0 != mode0) > convert_modes > > create_input_operand(..., xmode0) > > ? That seems more obvious than what you have. And I *think* > it should produce the same results. If it doesn't, then this > whole block of code needs a lot more explanation. The problem is that a VOIDmode match_operand doesn't necessary imply that "mode" is the right mode. VOIDmode register-accepting operands are only a warning, not an error, and are sometimes needed for flag-specific variations. They've traditionally not forced a conversion to "mode". E.g. if you have something like this on a 32-bit target: unsigned long long foo (unsigned long long x, int y) { return x >>= y; } then op1 will be (reg:SI y). Having a VOIDmode match_operand shouldn't force that to be converted to (reg:DI y), whereas I think the sequence above would. Or to put it another way: as things stand, "mode" is only trustworthy for CONST_INT opNs. A VOIDmode match_operand doesn't imply that the opN argument to expand_binop_directly is a CONST_INT, or even that only CONST_INTs are acceptable to the target. This comes back to the point that we really should know up-front what modes op0 and op1 actually have. (The thing I left as a future clean-up.) Until then, the process implemented by yesterday's patch was supposed to be: - work out what mode opN actually has at this point in time - see if the target has specifically asked for a different mode - if so, convert the operand This is directly equivalent to what create_convert_operand_from does: case EXPAND_CONVERT_FROM: if (GET_MODE (op->value) != VOIDmode) mode = GET_MODE (op->value); else /* The caller must tell us what mode this value has. */ gcc_assert (mode != VOIDmode); imode = insn_data[(int) icode].operand[opno].mode; if (imode != VOIDmode && imode != mode) { op->value = convert_modes (imode, mode, op->value, op->unsigned_p); mode = imode; } But we have to break the flow there (rather than go on to coerce the operands) because of the commutative thing. Richard