From patchwork Tue May 24 08:46:49 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Kugan Vivekanandarajah X-Patchwork-Id: 625542 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]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3rDTYq2ckvz9sp7 for ; Tue, 24 May 2016 18:47:26 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=fqpBDUKi; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; q=dns; s=default; b=yKZRZJttX4ekb8BlWC aCwSH+vQ1SlltrEvJYzQ9SD4qQ+hFFe17Xox63/fdv7P4vKo/fX9XIJnCvlJsAX2 PjLWHM2Y11opuK/APld3NSdKri3Nvr2gxoGm2EkP1/pLTuMUlZuzM7TNSj0Nhi+0 hKSdqYvrfxjZS4hN5I1XV630w= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; s=default; bh=1ihrHuuGPdGMnelr579FmzEY dC0=; b=fqpBDUKigyfIQZPTGqBwh3M4N+x5OXDag5QGv/Sw/FgW1nbeZD46DVSV sntgtRMYaIIZhSAQWgDQ6seYLsnKq+/rkd7BmGcc0TXhoGKQqA3KJpdZBHF+UsIc yg3EzI07+az/i0AZC1qphVg949w/OOFStJRgkLn5FeXgg1jJHuE= Received: (qmail 109581 invoked by alias); 24 May 2016 08:47:03 -0000 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 Received: (qmail 109568 invoked by uid 89); 24 May 2016 08:47:02 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-qg0-f45.google.com Received: from mail-qg0-f45.google.com (HELO mail-qg0-f45.google.com) (209.85.192.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 24 May 2016 08:46:52 +0000 Received: by mail-qg0-f45.google.com with SMTP id q32so4055869qgq.3 for ; Tue, 24 May 2016 01:46:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc; bh=Mbls2N4KPkvMUGAs0njDsTmmTEy/LAnhAp81tpzyAG4=; b=PSBx1dn8Qm3XjFZTnQcwORDP/0s2Q3krxY0P6F3bBOSTMaB05Nt5Y4KXjJGXNVayBI vG80fn7eE53/1haGTHz+rU4ZlN6Ea1mEa8rnTEvs/a3qs90ayoxe3MDfYUolDNrOgvCM dGr9dspn9l8XzYuywcy5/X8wj8Eg3pNfLlgObGxQjteQirRTs++7m6j0qjsaOHQB3IE1 RRmDwjBevPZRTbqH1z9OB1l+A1PeJ353Ci9IvPXWnSKqVSK54hIPj0GwicaIoBMP5Z+s zon0ZenO5D6cziITSpvMBlFyRCmRtAPXtUPV53R54xQijfwmvgqLhQLXkvkHI4rDZ/AK TK7A== X-Gm-Message-State: ALyK8tIj34dZ36TjKReqzVosGTc3aNoVKm9diX2gJs0XtCjWSp0TmfUh96ZDfXCk5BxwZFeggi6/da86tqNiPhKb MIME-Version: 1.0 X-Received: by 10.140.161.67 with SMTP id h64mr2177798qhh.96.1464079609396; Tue, 24 May 2016 01:46:49 -0700 (PDT) Received: by 10.200.42.71 with HTTP; Tue, 24 May 2016 01:46:49 -0700 (PDT) In-Reply-To: References: <573D7394.5050208@suse.cz> <573D78CE.6020900@linaro.org> Date: Tue, 24 May 2016 18:46:49 +1000 Message-ID: Subject: Re: [PATCH] Fix PR tree-optimization/71170 From: Kugan Vivekanandarajah To: Christophe Lyon Cc: Richard Biener , =?UTF-8?Q?Martin_Li=C5=A1ka?= , GCC Patches X-IsSubscribed: yes On 24 May 2016 at 18:36, Christophe Lyon wrote: > On 24 May 2016 at 05:13, Kugan Vivekanandarajah > wrote: >> On 23 May 2016 at 21:35, Richard Biener wrote: >>> On Sat, May 21, 2016 at 8:08 AM, Kugan Vivekanandarajah >>> wrote: >>>> On 20 May 2016 at 21:07, Richard Biener wrote: >>>>> On Fri, May 20, 2016 at 1:51 AM, Kugan Vivekanandarajah >>>>> wrote: >>>>>> Hi Richard, >>>>>> >>>>>>> I think it should have the same rank as op or op + 1 which is the current >>>>>>> behavior. Sth else doesn't work correctly here I think, like inserting the >>>>>>> multiplication not near the definition of op. >>>>>>> >>>>>>> Well, the whole "clever insertion" logic is simply flawed. >>>>>> >>>>>> What I meant to say was that the simple logic we have now wouldn’t >>>>>> work. "clever logic" is knowing where exactly where it is needed and >>>>>> inserting there. I think thats what you are suggesting below in a >>>>>> simple to implement way. >>>>>> >>>>>>> I'd say that ideally we would delay inserting the multiplication to >>>>>>> rewrite_expr_tree time. For example by adding a ops->stmt_to_insert >>>>>>> member. >>>>>>> >>>>>> >>>>>> Here is an implementation based on above. Bootstrap on x86-linux-gnu >>>>>> is OK. regression testing is ongoing. >>>>> >>>>> I like it. Please push the insertion code to a helper as I think you need >>>>> to post-pone setting the stmts UID to that point. >>>>> >>>>> Ideally we'd make use of the same machinery in attempt_builtin_powi, >>>>> removing the special-casing of powi_result. (same as I said that ideally >>>>> the plus->mult stuff would use the repeat-ops machinery...) >>>>> >>>>> I'm not 100% convinced the place you insert the stmt is correct but I >>>>> haven't spent too much time to decipher reassoc in this area. >>>> >>>> >>>> Hi Richard, >>>> >>>> Thanks. Here is a tested version of the patch. I did miss one place >>>> which I fixed now (tranform_stmt_to_copy) I also created a function to >>>> do the insertion. >>>> >>>> >>>> Bootstrap and regression testing on x86_64-linux-gnu are fine. Is this >>>> OK for trunk. >>> >>> @@ -3798,6 +3805,7 @@ rewrite_expr_tree (gimple *stmt, unsigned int opindex, >>> oe1 = ops[opindex]; >>> oe2 = ops[opindex + 1]; >>> >>> + >>> if (rhs1 != oe1->op || rhs2 != oe2->op) >>> { >>> gimple_stmt_iterator gsi = gsi_for_stmt (stmt); >>> >>> please remove this stray change. >>> >>> Ok with that change. >> >> Hi Richard, >> >> Thanks for the review. I also found another issue with this patch. >> I.e. for the stmt_to_insert we will get gimple_bb of NULL which is not >> expected in sort_by_operand_rank. This only showed up only while >> building a version of glibc. >> >> Bootstrap and regression testing are ongoing.Is this OK for trunk if >> passes regression and bootstrap. >> > > I'm seeing build failures in glibc after you committed r236619. > This new patch is fixing it, right? Yes (same patch attached). Also Bootstrap and regression testing on x86_64-linux-gnu didn’t have no new failures. Is this OK for trunk? Thanks, Kugan gcc/ChangeLog: 2016-05-24 Kugan Vivekanandarajah * tree-ssa-reassoc.c (sort_by_operand_rank): Check fgimple_bb for NULL. gcc/testsuite/ChangeLog: 2016-05-24 Kugan Vivekanandarajah * gcc.dg/tree-ssa/reassoc-44.c: New test. diff --git a/gcc/testsuite/gcc.dg/tree-ssa/reassoc-44.c b/gcc/testsuite/gcc.dg/tree-ssa/reassoc-44.c index e69de29..9b12212 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/reassoc-44.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/reassoc-44.c @@ -0,0 +1,10 @@ + +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +unsigned int a; +int b, c; +void fn1 () +{ + b = a + c + c; +} diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c index fb683ad..06f4d1b 100644 --- a/gcc/tree-ssa-reassoc.c +++ b/gcc/tree-ssa-reassoc.c @@ -525,7 +525,7 @@ sort_by_operand_rank (const void *pa, const void *pb) gimple *stmtb = SSA_NAME_DEF_STMT (oeb->op); basic_block bba = gimple_bb (stmta); basic_block bbb = gimple_bb (stmtb); - if (bbb != bba) + if (bba && bbb && bbb != bba) { if (bb_rank[bbb->index] != bb_rank[bba->index]) return bb_rank[bbb->index] - bb_rank[bba->index];