From patchwork Tue May 24 09:07:29 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Richard Biener X-Patchwork-Id: 625550 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 3rDV276gBqz9t0r for ; Tue, 24 May 2016 19:08:29 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=a6465onL; 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:content-transfer-encoding; q=dns; s= default; b=VzpGmiNiEzf5pVNlJGAoVZL0YkrBlZQeqeeRi2EjqtOoVgFcVD1Fc tJVItZpMbnjFJ83OZosqlSkofinS/UZO/yaqP2VheQ0QRnx7UvbSV0pzB6FSoX5/ YK4LLYpTMc0zirOon0ofru3fYl+AQqAeazWDtfzVDDq64bqd4EcqVw= 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:content-transfer-encoding; s=default; bh=GzWQisrNbJJ9qm07hVF4Rm/hEeI=; b=a6465onLs0F0yb2A8iCOGBEiu9GG MzlzZbsjx6a722TG82nlb6VmM1gfEH3eLtn78FK34GY/IfA2pFJylXMNUW1W/W5n 0D29u6AjBdF+JKvSZZ7rbON/KpahHse2iMUBiqyBnDyLb36tSyFHL0N5+3faY6eE t0gW/uexTwJ3n40= Received: (qmail 28298 invoked by alias); 24 May 2016 09:07:43 -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 28279 invoked by uid 89); 24 May 2016 09:07:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy=welldefined, well-defined, 5196, thru X-HELO: mail-wm0-f42.google.com Received: from mail-wm0-f42.google.com (HELO mail-wm0-f42.google.com) (74.125.82.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 24 May 2016 09:07:32 +0000 Received: by mail-wm0-f42.google.com with SMTP id a136so62126696wme.0 for ; Tue, 24 May 2016 02:07:32 -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:content-transfer-encoding; bh=G8b0EHwJwDUEe9ev5oJluBwCuU6iqAaM0b9FA7iMPMI=; b=NWNR+oKkvPCPV83OX0gczm19dbisOcBkAr1cKkqcacOK6eFtjl7yCiyFoQ+qSwCl2e bZ10PrE/sRw1dfMGE6jTOk+6P6FkIO8/kDQiZCQLfzchcTG/FaB4j554VZ7rk/g11qdI ei1jzdWn6FnOCJL3slhUBQGcifCAGvEVdbyCLL0cuf6pwytZX3NM6JAGoftboAZ9aWrO bHV13br6EaQEHgJFa1t597gLak0LgG85Vn0/ZPzc8adbScy9WGde/pkjrZulQCLxjH7X JKfNuN9YVbw3gYzdvKdGBgC7ll82KhAA1Eq4uLPvNnq/8rgfsASM7jYivP7a6j4eMLx3 kPFg== X-Gm-Message-State: ALyK8tJhv/CFXAakq+IavyPW5Bly2uo09WCcKF0V3DDjikbACcLChvLQmbQ7ZBQaKSgQMPiiW7K6CT1XCqQyfg== MIME-Version: 1.0 X-Received: by 10.194.223.70 with SMTP id qs6mr3060902wjc.119.1464080849711; Tue, 24 May 2016 02:07:29 -0700 (PDT) Received: by 10.194.87.34 with HTTP; Tue, 24 May 2016 02:07:29 -0700 (PDT) In-Reply-To: References: <573D7394.5050208@suse.cz> <573D78CE.6020900@linaro.org> Date: Tue, 24 May 2016 11:07:29 +0200 Message-ID: Subject: Re: [PATCH] Fix PR tree-optimization/71170 From: Richard Biener To: Kugan Vivekanandarajah Cc: =?UTF-8?Q?Martin_Li=C5=A1ka?= , GCC Patches X-IsSubscribed: yes On Tue, May 24, 2016 at 5:13 AM, 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. Hmm, I'd rather fall thru to the SSA_NAME_VERSION or id comparison here than to stmt_dominates_stmt which is only well-defined for stmts in the same BB. So sth like ok with that change. Richard. > 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. Index: gcc/tree-ssa-reassoc.c =================================================================== --- gcc/tree-ssa-reassoc.c (revision 236630) +++ gcc/tree-ssa-reassoc.c (working copy) @@ -519,6 +519,8 @@ sort_by_operand_rank (const void *pa, co See PR60418. */ if (!SSA_NAME_IS_DEFAULT_DEF (oea->op) && !SSA_NAME_IS_DEFAULT_DEF (oeb->op) + && !oea->stmt_to_insert + && !oeb->stmt_to_insert && SSA_NAME_VERSION (oeb->op) != SSA_NAME_VERSION (oea->op)) { gimple *stmta = SSA_NAME_DEF_STMT (oea->op);