From patchwork Tue May 24 03:13:32 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Kugan Vivekanandarajah X-Patchwork-Id: 68433 Delivered-To: patch@linaro.org Received: by 10.140.92.199 with SMTP id b65csp403882qge; Mon, 23 May 2016 20:14:11 -0700 (PDT) X-Received: by 10.66.222.98 with SMTP id ql2mr3138701pac.137.1464059651663; Mon, 23 May 2016 20:14:11 -0700 (PDT) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id fl7si1324733pac.217.2016.05.23.20.14.11 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 23 May 2016 20:14:11 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-return-428100-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org; spf=pass (google.com: domain of gcc-patches-return-428100-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-428100-patch=linaro.org@gcc.gnu.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org 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=n5Kv1wapW9oN8FslYI UrGa3xVNaKwCCOLXKyr49MDOROPDHvQqTc5iP5CysJQIvnxSStPpPQKCHwFrunWu Om4DqXGGRNpnAggZrBCvalGXUaHNPM64XNZFQu1zgV9YQWQfn8Vf/5KgI4AAhG8g 8Wob89TM2904g874OWMOpUuE4= 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=Tn0nu+ItL4QTjuJtAS9RsMgz gyk=; b=hnSLhyShaNr679HXqgypB5/HYL2a5Zfs2gNrXxCDz9DV9c9rvOG24Sww NLatYngJ+fv/KANrppDqgXUn8G3Bd1nsMOdWHLW2KKQ0V+h+Zwc8N8q0paPH1SUb 6sqko0E6x23SoGK8tHKM4F/h0q19sOhxk1Y/Rv2aBsXBE4iOPds= Received: (qmail 93901 invoked by alias); 24 May 2016 03:13:47 -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 92899 invoked by uid 89); 24 May 2016 03:13:45 -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=bba, 2016-05-24, 20160524 X-HELO: mail-qk0-f181.google.com Received: from mail-qk0-f181.google.com (HELO mail-qk0-f181.google.com) (209.85.220.181) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 24 May 2016 03:13:35 +0000 Received: by mail-qk0-f181.google.com with SMTP id n63so2942370qkf.0 for ; Mon, 23 May 2016 20:13:35 -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=jhu5Sd/xxbLP7AHbxKTrFAjkX4yGQexJB3RF9mHdZiU=; b=WYN0DlPDcuU9PPwKv+mv0z9f2ZDF+M6jsE56RFWC1BNvqbpoCs1sxPI0yw0P7nZ+hs UvwR9hFj/hd1sWtBns0JATolVmH8rzg0uBJgqb9EAQ5cuyFA9absv8U6kEKq6Yve4gFv nM3BTl/wQ4JrwCnobx2fYeCmrjuzCf8IWGvMj6Ocje32rICER2Ru/MvNdKOL+Lejgs9t 5d+/aHNaIiPiHiafY/FOdCOuGVp4Zaq1C0zp288AGJsSOyx5gWia/wiNcv/N/FL1okwy zVTT5XT1WdZLPHo8HVy/XKzO5JkqS7GJCDRSdc5yv4Pl97v00njLkYB6Y5AWfM2ldJPb BMPw== X-Gm-Message-State: ALyK8tLQDfTVISWb292+k7Z7G3ufp5x/u3ifxlDsLaCo3sT7NSxo/eqPU3FnXXoxyzR9ajGU7Il6AaTPGNf02wj7 MIME-Version: 1.0 X-Received: by 10.237.34.240 with SMTP id q45mr2041994qtc.90.1464059612447; Mon, 23 May 2016 20:13:32 -0700 (PDT) Received: by 10.200.42.71 with HTTP; Mon, 23 May 2016 20:13:32 -0700 (PDT) In-Reply-To: References: <573D7394.5050208@suse.cz> <573D78CE.6020900@linaro.org> Date: Tue, 24 May 2016 13:13:32 +1000 Message-ID: Subject: Re: [PATCH] Fix PR tree-optimization/71170 From: Kugan Vivekanandarajah To: Richard Biener Cc: =?UTF-8?Q?Martin_Li=C5=A1ka?= , GCC Patches X-IsSubscribed: yes 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. Thanks, Kugan gcc/ChangeLog: 2016-05-24 Kugan Vivekanandarajah * tree-ssa-reassoc.c (sort_by_operand_rank): Check for gimple_bb of NULL for stmt_to_insert. 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];