From patchwork Wed Jul 28 20:15:41 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Sandiford X-Patchwork-Id: 60175 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 22580B70A4 for ; Thu, 29 Jul 2010 06:16:01 +1000 (EST) Received: (qmail 22574 invoked by alias); 28 Jul 2010 20:15:58 -0000 Received: (qmail 22562 invoked by uid 22791); 28 Jul 2010 20:15:55 -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, TW_TM, T_TO_NO_BRKTS_FREEMAIL 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, 28 Jul 2010 20:15:48 +0000 Received: by wyb38 with SMTP id 38so5149123wyb.20 for ; Wed, 28 Jul 2010 13:15:46 -0700 (PDT) Received: by 10.227.157.147 with SMTP id b19mr9915255wbx.49.1280348145899; Wed, 28 Jul 2010 13:15:45 -0700 (PDT) Received: from localhost (rsandifo.gotadsl.co.uk [82.133.89.107]) by mx.google.com with ESMTPS id i25sm5698140wbi.10.2010.07.28.13.15.43 (version=TLSv1/SSLv3 cipher=RC4-MD5); Wed, 28 Jul 2010 13:15:44 -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> Date: Wed, 28 Jul 2010 21:15:41 +0100 In-Reply-To: <1280331613.18689.137.camel@e102325-lin.cambridge.arm.com> (Ramana Radhakrishnan's message of "Wed, 28 Jul 2010 16:40:13 +0100") Message-ID: <87vd7zpdle.fsf@firetop.home> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 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 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). 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;