From patchwork Thu Jul 29 10:33:27 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ramana Radhakrishnan X-Patchwork-Id: 60202 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 38DF6B70AA for ; Thu, 29 Jul 2010 20:33:48 +1000 (EST) Received: (qmail 11167 invoked by alias); 29 Jul 2010 10:33:46 -0000 Received: (qmail 11154 invoked by uid 22791); 29 Jul 2010 10:33:41 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL, BAYES_00, TW_TM, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from cam-admin0.cambridge.arm.com (HELO cam-admin0.cambridge.arm.com) (217.140.96.50) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 29 Jul 2010 10:33:34 +0000 Received: from cam-owa1.Emea.Arm.com (cam-owa1.emea.arm.com [10.1.255.62]) by cam-admin0.cambridge.arm.com (8.12.6/8.12.6) with ESMTP id o6TAXQF9016966; Thu, 29 Jul 2010 11:33:26 +0100 (BST) Received: from [10.1.66.29] ([10.1.255.212]) by cam-owa1.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.0); Thu, 29 Jul 2010 11:33:27 +0100 Subject: Re: Extend widening_mul pass to handle fixed-point types From: Ramana Radhakrishnan Reply-To: ramana.radhakrishnan@arm.com To: Richard Sandiford Cc: gcc-patches@gcc.gnu.org, bernds@codesourcery.com In-Reply-To: <87vd7zpdle.fsf@firetop.home> 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: Thu, 29 Jul 2010 11:33:27 +0100 Message-Id: <1280399607.10110.33.camel@e102325-lin.cambridge.arm.com> Mime-Version: 1.0 X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Hi Richard, On Wed, 2010-07-28 at 21:15 +0100, Richard Sandiford wrote: > 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. Thanks for looking at this. Ah - ok. I see what you mean here. > > > 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). Thanks for the clarification , that makes more sense now. > > 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). Your patch didn't apply cleanly to pristine trunk or to 162431 and I had to do a manual merge so I'm not sure if I got all parts of it. Just to be absolutely clear, you still need to check for is_gimple_assign before using the stmt in gimple_assign_lhs in is_widening_mult_p or do you expect that to get subsumed by your patch ? Otherwise the compiler ICEs with this testcase [ice-on-gimple-phi.c]. Here's what I came up with that includes a manual merge of your patch assuming I got all the hunks, and my original trivial hunk to is_widening_mult_p which appeared to fix all the problems with simple testcases on trunk with a cross-compiler and what I'm bootstrapping and testing right now. cheers Ramana 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. (is_widening_mult_p): Return false if stmt is not a GIMPLE_ASSIGN. [1] ./xgcc -B`pwd` -S -O2 -mcpu=cortex-a9 ~/ice-on-gimple-phi.c assignment./home/ramrad01/ice-on-gimple-phi.c:57:1: internal compiler error: gimple check: expected gimple_assign(error_mark), have gimple_phi() in gimple_assign_lhs, at gimple.h:1724 > > 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; Index: tree-ssa-math-opts.c =================================================================== --- tree-ssa-math-opts.c (revision 162431) +++ tree-ssa-math-opts.c (working copy) @@ -1323,6 +1323,9 @@ is_widening_mult_p (gimple stmt, { tree type; + if (!is_gimple_assign (stmt)) + return false; + type = TREE_TYPE (gimple_assign_lhs (stmt)); if (TREE_CODE (type) != INTEGER_TYPE && TREE_CODE (type) != FIXED_POINT_TYPE) @@ -1414,13 +1417,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,8 +1443,6 @@ 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) @@ -1456,14 +1450,14 @@ 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 = 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) @@ -1471,13 +1465,27 @@ convert_plusminus_to_widen (gimple_stmt_ mult_rhs1 = gimple_assign_rhs1 (rhs2_stmt); mult_rhs2 = gimple_assign_rhs2 (rhs2_stmt); add_rhs = rhs1; + type1 = TREE_TYPE (mult_rhs1); + type2 = TREE_TYPE (mult_rhs2); } 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;