From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 89474 invoked by alias); 20 Sep 2016 11:44:34 -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 89462 invoked by uid 89); 20 Sep 2016 11:44:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=ham version=3.3.2 spammy=Take X-HELO: mail-wm0-f41.google.com Received: from mail-wm0-f41.google.com (HELO mail-wm0-f41.google.com) (74.125.82.41) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 20 Sep 2016 11:44:23 +0000 Received: by mail-wm0-f41.google.com with SMTP id w84so128912289wmg.1 for ; Tue, 20 Sep 2016 04:44:22 -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:content-transfer-encoding; bh=w5A0OFmV0TGM67f34K47tMMRk1C2irAzW95cqmWL5MM=; b=WqmuQF7+stYSeB0yOrcmt6wm4HGmC/Cy7p4n2mu09p/UU0rTosfTDuuP9c7aAMWxKZ z/aNeaZHzPULXbKEl2ojgDWMGwHQ/KOI3rcPoLwbumatV8yN3uvG37xRzyjgn9EmjOEF oH97oFiaD1gtMiolHPq0YskvQjNLXNEX0QPAbYRpJRA4jQcl1EFkwJPRF20Zi5SLjhFA 7nmUYcVzRG/MXAPNcNz66pEf5H+UdukisZbM6c5E7FPKgbcPkx6GhcwCm8bsCMRnGLsu fTw8FTI6AlHE22fVVrqrp26jO2V+2zl481OVHzhehqPS/Rt1Xthy5tcwrUJefWoFIKPH 8R/g== X-Gm-Message-State: AE9vXwMT7MeMw6ZcAmiEWotMuAslweMtngH/uuSokZ4yYoiXnw+A3De8nlGShKzj7sG7belJKA3E7LaQ0iKDww== X-Received: by 10.194.88.137 with SMTP id bg9mr27437110wjb.155.1474371860866; Tue, 20 Sep 2016 04:44:20 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.137.129 with HTTP; Tue, 20 Sep 2016 04:44:20 -0700 (PDT) In-Reply-To: <2492940f-f36c-3507-178a-8e215536f28d@linaro.org> 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> <2492940f-f36c-3507-178a-8e215536f28d@linaro.org> From: Richard Biener Date: Tue, 20 Sep 2016 12:01: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 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2016-09/txt/msg01276.txt.bz2 On Tue, Sep 20, 2016 at 1:32 AM, kugan wrote: > 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=3Dfalse 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 wi= ll >>>>>>> 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 =3D 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 =3D 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-chain= s. >>>>>> Take their subops, simulate zero_one_operation by >>>>>> appending the cancelling operation and optimizing the oplist, and th= en >>>>>> 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) =3D=3D 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) =3D=3D op) >>>> { >>>> tree cst =3D 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 =E2=80=98auto_vec&=E2=80=99 from > expression of type =E2=80=98auto_vec > > Is this a bug? You need to use vec, not auto_vec. Richard. > Thanks, > Kugan