From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25819 invoked by alias); 25 Aug 2016 12:24:15 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 25808 invoked by uid 89); 25 Aug 2016 12:24:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=associates, sensitive, cif, multiplication 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 ESMTP; Thu, 25 Aug 2016 12:24:04 +0000 Received: by mail-wm0-f42.google.com with SMTP id i5so69133250wmg.0 for ; Thu, 25 Aug 2016 05:24:03 -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:from:date :message-id:subject:to:cc; bh=Et/wy/ZmConI6jB4LbncFTjuQgOF05WqXAkwskFb8SQ=; b=Zb9M7vmGdFmYOIi4a3Gv7XodlvlR8tFSH9zMXS1s/Ia+a3hWtixVLy0NFCyvSfiPmM 7eheoI/d8AtYpmwi088BONdvz/iHxAwVvibzsmT3iCkFyhBIcH2U0LPJevIWA4qyMMVY DijwlYcGf6zs7cqTZEEskDKDtschXa7r+6j1Ob0JKAn/Lx0IelNlcsEad+9KxFDenzQ3 QAhjysNdwZdKGq/Gq9nh2LBeLWvSfYy/2oOjSoe4TqsXG9PzFMYsuT9XIZzpPeERvqIe L58r4eJsjxOEHeRCmb157/A44wCEpKj6ykYydYCmbNhwhRtFVZrhCfUWaZ9sJPQaao4E QFlQ== X-Gm-Message-State: AE9vXwNYJyz5tDCtEWMbMLxTIhCo7NSfiEhEj4zfuReMU1jyJtZ1t8V8Mxu2R1lH170Iss39T4KpOEmPwHRMww== X-Received: by 10.28.35.86 with SMTP id j83mr7591578wmj.18.1472127841791; Thu, 25 Aug 2016 05:24:01 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.137.202 with HTTP; Thu, 25 Aug 2016 05:24:00 -0700 (PDT) In-Reply-To: References: <0a1eaaf8-3ede-cd56-ffb5-40b25f94e46e@linaro.org> <98613cff-7c48-1a56-0014-6d87c35a8f26@linaro.org> <20160809214617.GB14857@tucnak.redhat.com> <7210cceb-be3b-44b1-13b7-4152e89d2a4f@linaro.org> <20160809215527.GC14857@tucnak.redhat.com> <0c53b0f3-4af6-387c-9350-95b1ae85850d@linaro.org> <20160810085703.GH14857@tucnak.redhat.com> From: Richard Biener Date: Thu, 25 Aug 2016 12:24:00 -0000 Message-ID: Subject: Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments To: kugan Cc: Jakub Jelinek , "gcc-patches@gcc.gnu.org" Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2016-08/txt/msg01799.txt.bz2 On Thu, Aug 11, 2016 at 1:09 AM, kugan wrote: > Hi, > > > On 10/08/16 20:28, Richard Biener wrote: >> >> On Wed, Aug 10, 2016 at 10:57 AM, Jakub Jelinek wrote: >>> >>> On Wed, Aug 10, 2016 at 08:51:32AM +1000, kugan wrote: >>>> >>>> I see it now. The problem is we are just looking at (-1) being in the >>>> ops >>>> list for passing changed to rewrite_expr_tree in the case of >>>> multiplication >>>> by negate. If we have combined (-1), as in the testcase, we will not >>>> have >>>> the (-1) and will pass changed=false to rewrite_expr_tree. >>>> >>>> We should set changed based on what happens in try_special_add_to_ops. >>>> Attached patch does this. Bootstrap and regression testing are ongoing. >>>> Is >>>> this OK for trunk if there is no regression. >>> >>> >>> I think the bug is elsewhere. In particular in >>> undistribute_ops_list/zero_one_operation/decrement_power. >>> All those look problematic in this regard, they change RHS of statements >>> to something that holds a different value, while keeping the LHS. >>> So, generally you should instead just add a new stmt next to the old one, >>> and adjust data structures (replace the old SSA_NAME in some ->op with >>> the new one). decrement_power might be a problem here, dunno if all the >>> builtins are const in all cases that DSE would kill the old one, >>> Richard, any preferences for that? reset flow sensitive info + reset >>> debug >>> stmt uses, or something different? Though, replacing the LHS with a new >>> anonymous SSA_NAME might be needed too, in case it is before SSA_NAME of >>> a >>> user var that doesn't yet have any debug stmts. >> >> >> I'd say replacing the LHS is the way to go, with calling the appropriate >> helper >> on the old stmt to generate a debug stmt for it / its uses (would need >> to look it >> up here). >> > > Here is an attempt to fix it. The problem arises when in > undistribute_ops_list, we linearize_expr_tree such that NEGATE_EXPR is added > (-1) MULT_EXPR (OP). Real problem starts when we handle this in > zero_one_operation. Unlike what was done earlier, we now change the stmt > (with propagate_op_to_signle use or by directly) such that the value > computed by stmt is no longer what it used to be. Because of this, what is > computed in undistribute_ops_list and rewrite_expr_tree are also changed. > > undistribute_ops_list already expects this but rewrite_expr_tree will not if > we dont pass the changed as an argument. > > The way I am fixing this now is, in linearize_expr_tree, I set ops_changed > to true if we change NEGATE_EXPR to (-1) MULT_EXPR (OP). Then when we call > zero_one_operation with ops_changed = true, I replace all the LHS in > zero_one_operation with the new SSA and replace all the uses. I also call > the rewrite_expr_tree with changed = false in this case. > > Does this make sense? Bootstrapped and regression tested for > x86_64-linux-gnu without any new regressions. I don't think this solves the issue. zero_one_operation associates the chain starting at the first *def and it will change the intermediate values of _all_ of the stmts visited until the operation to be removed is found. Note that this is independent of whether try_special_add_to_ops did anything. Even for the regular undistribution cases we get this wrong. So we need to back-track in zero_one_operation, replacing each LHS and in the end the op in the opvector of the main chain. That's basically the same as if we'd do a regular re-assoc operation on the sub-chains. Take their subops, simulate zero_one_operation by appending the cancelling operation and optimizing the oplist, and then materializing the associated ops via rewrite_expr_tree. Richard. > Thanks, > Kugan > > > gcc/testsuite/ChangeLog: > > 2016-08-10 Kugan Vivekanandarajah > > PR tree-optimization/72835 > * gcc.dg/tree-ssa/pr72835.c: New test. > > gcc/ChangeLog: > > 2016-08-10 Kugan Vivekanandarajah > > PR tree-optimization/72835 > * tree-ssa-reassoc.c (zero_one_operation): Incase of NEGATE_EXPR > create and use > new SSA_NAME. > (try_special_add_to_ops): Return true if we changed the value in > operands. > (linearize_expr_tree): Return true if try_special_add_top_ops set > ops_changed to true. > (undistribute_ops_list): Likewise. > (reassociate_bb): Pass ops_changed returned by linearlize_expr_tree > to rewrite_expr_tree. > > > > whil cif we change the operands such that the > > /zero_one_operation