From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17115 invoked by alias); 19 Sep 2016 23:33:10 -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 17081 invoked by uid 89); 19 Sep 2016 23:33:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=Take X-HELO: mail-pa0-f52.google.com Received: from mail-pa0-f52.google.com (HELO mail-pa0-f52.google.com) (209.85.220.52) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 19 Sep 2016 23:32:58 +0000 Received: by mail-pa0-f52.google.com with SMTP id hm5so245132pac.0 for ; Mon, 19 Sep 2016 16:32:58 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=4VC+IWx9jQz7bcDX1753lzWAx35O3seagNAB9uU8fWU=; b=U5IzJcm1V/H756siYCsP5tVHbNszIiK1suIWZzWmj6AoO3KwxdA4Ol5kCUZ9MSHlD3 /aUA6h1K22hEBkN8W/uWd5/ImA7YvJS/hSBJdzz66e44UTw3/E18uUCa9mQ2bu83qycE kiWKZZgC18rkWEAjVuy+nxLFyEv29v7vmaCr1MNA+vilQT/ny83MvYQX68WG5oD/kjvw e39/LQTWagRnhSMC6lCzh1cH489qrpNAxrwL0cLwI0QDwjPigDotaOWgtQnX3UyLTFWj UQ8dO4283f5aKudfI2I2F8roRSaIYeVY1AQd1CQ6n9xWPdxWdaEcmQaGI99/WA9a9M+M GKHg== X-Gm-Message-State: AE9vXwMkriRs/hEnq9w9WTwnf+G5CMeHcDbj7fghFgWWrfC9jT9xmDg0ilR1NNQRRPWHOsQ5 X-Received: by 10.67.8.41 with SMTP id dh9mr50699270pad.132.1474327977054; Mon, 19 Sep 2016 16:32:57 -0700 (PDT) Received: from [10.1.1.7] (58-6-183-210.dyn.iinet.net.au. [58.6.183.210]) by smtp.gmail.com with ESMTPSA id b69sm11907844pfb.55.2016.09.19.16.32.54 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 19 Sep 2016 16:32:55 -0700 (PDT) Subject: Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments To: Richard Biener 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> <0f3b4359-f5ff-d14c-1b15-2ae647e6fd3a@linaro.org> Cc: Jakub Jelinek , "gcc-patches@gcc.gnu.org" From: kugan Message-ID: <2492940f-f36c-3507-178a-8e215536f28d@linaro.org> Date: Tue, 20 Sep 2016 03:27:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2016-09/txt/msg01252.txt.bz2 Hi Richard, Thanks for the review. On 19/09/16 23:40, Richard Biener wrote: > On Sun, Sep 18, 2016 at 10:21 PM, kugan > wrote: >> Hi Richard, >> >> >> On 14/09/16 21:31, Richard Biener wrote: >>> >>> On Fri, Sep 2, 2016 at 10:09 AM, Kugan Vivekanandarajah >>> wrote: >>>> >>>> Hi Richard, >>>> >>>> On 25 August 2016 at 22:24, Richard Biener >>>> wrote: >>>>> >>>>> 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. >>>>> >>>> Here is a draft patch which records the stmt chain when in >>>> zero_one_operation and then fixes it when OP is removed. when we >>>> update *def, that will update the ops vector. Does this looks sane? >>> >>> >>> Yes. A few comments below >>> >>> + /* PR72835 - Record the stmt chain that has to be updated such that >>> + we dont use the same LHS when the values computed are different. */ >>> + auto_vec stmts_to_fix; >>> >>> use auto_vec here so we get stack allocation only most >>> of the times >> >> Done. >> >>> if (stmt_is_power_of_op (stmt, op)) >>> { >>> + make_new_ssa_for_all_defs (def, op, stmts_to_fix); >>> if (decrement_power (stmt) == 1) >>> propagate_op_to_single_use (op, stmt, def); >>> >>> for the cases you end up with propagate_op_to_single_use its argument >>> stmt is handled superfluosly in the new SSA making, I suggest to pop it >>> from the stmts_to_fix vector in that case. I suggest to break; instead >>> of return in all cases and do the make_new_ssa_for_all_defs call at >>> the function end instead. >>> >> Done. >> >>> @@ -1253,14 +1305,18 @@ zero_one_operation (tree *def, enum tree_code >>> opcode, tree op) >>> if (gimple_assign_rhs1 (stmt2) == op) >>> { >>> tree cst = build_minus_one_cst (TREE_TYPE (op)); >>> + stmts_to_fix.safe_push (stmt2); >>> + make_new_ssa_for_all_defs (def, op, stmts_to_fix); >>> propagate_op_to_single_use (cst, stmt2, def); >>> return; >>> >>> this safe_push should be unnecessary for the above reason (others are >>> conditionally unnecessary). >>> >> Done. >> >> Bootstrapped and regression tested on X86_64-linux-gnu with no new >> regression. Is this OK? > > +static void > +make_new_ssa_for_all_defs (tree *def, tree op, > + auto_vec &stmts_to_fix) > > I think you need to use vec &stmts_to_fix here AFAIK. > This is what I had. With that I get: error: invalid initialization of reference of type ‘auto_vec&’ from expression of type ‘auto_vec Is this a bug? Thanks, Kugan