From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3285 invoked by alias); 31 Jul 2010 07:30:28 -0000 Received: (qmail 3252 invoked by uid 22791); 31 Jul 2010 07:30:26 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,TW_TM,T_TO_NO_BRKTS_FREEMAIL X-Spam-Check-By: sourceware.org Received: from mail-ww0-f51.google.com (HELO mail-ww0-f51.google.com) (74.125.82.51) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 31 Jul 2010 07:30:17 +0000 Received: by wwe15 with SMTP id 15so1292478wwe.8 for ; Sat, 31 Jul 2010 00:30:14 -0700 (PDT) Received: by 10.227.127.65 with SMTP id f1mr2655857wbs.118.1280561414631; Sat, 31 Jul 2010 00:30:14 -0700 (PDT) Received: from localhost (rsandifo.gotadsl.co.uk [82.133.89.107]) by mx.google.com with ESMTPS id f30sm2727146wbe.18.2010.07.31.00.30.13 (version=TLSv1/SSLv3 cipher=RC4-MD5); Sat, 31 Jul 2010 00:30:13 -0700 (PDT) From: Richard Sandiford To: ramana.radhakrishnan@arm.com Mail-Followup-To: ramana.radhakrishnan@arm.com,Bernd Schmidt , gcc-patches@gcc.gnu.org, rdsandiford@googlemail.com Cc: Bernd Schmidt , gcc-patches@gcc.gnu.org Subject: Re: Extend widening_mul pass to handle fixed-point types References: <87fwzhro8i.fsf@firetop.home> <4C436835.20307@codesourcery.com> <87oce3s4kv.fsf@firetop.home> <4C44C948.3050801@codesourcery.com> <1280233130.18689.17.camel@e102325-lin.cambridge.arm.com> <4C4F009B.6040201@codesourcery.com> <1280331613.18689.137.camel@e102325-lin.cambridge.arm.com> <87vd7zpdle.fsf@firetop.home> Date: Sat, 31 Jul 2010 09:45:00 -0000 In-Reply-To: <87vd7zpdle.fsf@firetop.home> (Richard Sandiford's message of "Wed, 28 Jul 2010 21:15:41 +0100") Message-ID: <877hkcf6re.fsf@firetop.home> 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: 2010-07/txt/msg02404.txt.bz2 Richard Sandiford writes: > Ramana Radhakrishnan writes: >> Hmmm there are regressions after my patch was applied. I had to do some >> rebuilds because trunk appears to be broken for other reasons at the >> minute for ARM which I shall investigate next. > > Yeah, I think it's the other way about: expand_widen_pattern_expr > is using the right type and convert_plusminus_to_widen is using > the wrong one. The type we pass to optab_for_tree_code determines > the _sign_ of the multiplication, not the size, so we want to pass > in the unwidened type. > >> We weren't generating any widening operations before this patch (and I >> suspect my 2 patches are just for fixing the ICE's and doing the right >> thing). If it were just adding support for fixed point arithmetic then >> this is just broken because it shouldn't have changed the way in which >> code was generated for normal integer operations. > > For the record, the patch wasn't just about adding fixed-point support. > It was also about allowing multiply-accumulate to be used where no plain > multiplication exists. So the problem for ARM was that the use of the > wrong type in convert_plusminus_to_widen was being hidden by the use of > the right type in convert_mult_widen (i.e. the patch exposed a latent bug). > > Could you give the attached patch a try? It also fixes a case where > the code could create mixed sign-and-unsigned maccs (a bug that existed > before my patch) and a botched call to is_widening_mult_p (a bug > introduced by my patch, sorry). Now bootstrapped & regression-tested on x86_64-linux-gnu. Ramana also confirms that it fixes the ICEs and incorrect arith-rand-ll.c code on ARM. OK to install? Richard > gcc/ > * tree-ssa-math-opts.c (convert_plusminus_to_widen): Fix type > used in the call to optab_for_tree_code. Fix the second > is_widening_mult_p call. Check that both unwidened operands > have the same sign. > > Index: gcc/tree-ssa-math-opts.c > =================================================================== > --- gcc/tree-ssa-math-opts.c 2010-07-28 21:09:59.000000000 +0100 > +++ gcc/tree-ssa-math-opts.c 2010-07-28 21:10:00.000000000 +0100 > @@ -1414,13 +1414,6 @@ convert_plusminus_to_widen (gimple_stmt_ > else > wmult_code = WIDEN_MULT_PLUS_EXPR; > > - /* Verify that the machine can perform a widening multiply > - accumulate in this mode/signedness combination, otherwise > - this transformation is likely to pessimize code. */ > - this_optab = optab_for_tree_code (wmult_code, type, optab_default); > - if (optab_handler (this_optab, TYPE_MODE (type)) == CODE_FOR_nothing) > - return false; > - > rhs1 = gimple_assign_rhs1 (stmt); > rhs2 = gimple_assign_rhs2 (stmt); > > @@ -1447,37 +1440,49 @@ convert_plusminus_to_widen (gimple_stmt_ > if (!is_widening_mult_p (rhs1_stmt, &type1, &mult_rhs1, > &type2, &mult_rhs2)) > return false; > - mult_rhs1 = fold_convert (type1, mult_rhs1); > - mult_rhs2 = fold_convert (type2, mult_rhs2); > add_rhs = rhs2; > } > else if (rhs2_code == MULT_EXPR) > { > - if (!is_widening_mult_p (rhs1_stmt, &type1, &mult_rhs1, > + if (!is_widening_mult_p (rhs2_stmt, &type1, &mult_rhs1, > &type2, &mult_rhs2)) > return false; > - mult_rhs1 = fold_convert (type1, mult_rhs1); > - mult_rhs2 = fold_convert (type2, mult_rhs2); > add_rhs = rhs1; > } > else if (code == PLUS_EXPR && rhs1_code == WIDEN_MULT_EXPR) > { > mult_rhs1 = gimple_assign_rhs1 (rhs1_stmt); > mult_rhs2 = gimple_assign_rhs2 (rhs1_stmt); > + type1 = TREE_TYPE (mult_rhs1); > + type2 = TREE_TYPE (mult_rhs2); > add_rhs = rhs2; > } > else if (rhs2_code == WIDEN_MULT_EXPR) > { > mult_rhs1 = gimple_assign_rhs1 (rhs2_stmt); > mult_rhs2 = gimple_assign_rhs2 (rhs2_stmt); > + type1 = TREE_TYPE (mult_rhs1); > + type2 = TREE_TYPE (mult_rhs2); > add_rhs = rhs1; > } > else > return false; > > + if (TYPE_UNSIGNED (type1) != TYPE_UNSIGNED (type2)) > + return false; > + > + /* Verify that the machine can perform a widening multiply > + accumulate in this mode/signedness combination, otherwise > + this transformation is likely to pessimize code. */ > + this_optab = optab_for_tree_code (wmult_code, type1, optab_default); > + if (optab_handler (this_optab, TYPE_MODE (type)) == CODE_FOR_nothing) > + return false; > + > /* ??? May need some type verification here? */ > > - gimple_assign_set_rhs_with_ops_1 (gsi, wmult_code, mult_rhs1, mult_rhs2, > + gimple_assign_set_rhs_with_ops_1 (gsi, wmult_code, > + fold_convert (type1, mult_rhs1), > + fold_convert (type2, mult_rhs2), > add_rhs); > update_stmt (gsi_stmt (*gsi)); > return true;