From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 77476 invoked by alias); 19 Jul 2017 18:13:30 -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 77455 invoked by uid 89); 19 Jul 2017 18:13:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-10.6 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-wm0-f45.google.com Received: from mail-wm0-f45.google.com (HELO mail-wm0-f45.google.com) (74.125.82.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 19 Jul 2017 18:13:26 +0000 Received: by mail-wm0-f45.google.com with SMTP id g127so6456169wmd.0 for ; Wed, 19 Jul 2017 11:13:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:user-agent:in-reply-to:references :mime-version:content-transfer-encoding:subject:to:cc:from :message-id; bh=rxS9Jhv4y4SbZD8uKME1UP0oeC5aqNqhoWmpBcBuwPA=; b=s7g1h0TIHQVMUVJ9LrMRQS4wqyU4tVtU0eHMTmsIGSM2SX28l/LLUIVZ+2pGfpIeXS nCqIAMtRuSvHM4X0K8bwdRzql1Ko17yeDmfE9OVk/VCenYjLhaL7iC2BWeGyKxywnY7b mMdz2xj599/OoLBpUGu+kCNqnGrVcZ3sLPrqKLcTwcdi8swkaiMuegXxYUDb5Sihbu3Z 25AgpqwGznddUzetMWGYkz5pklR03tEA9BFVY8xWOH0VrGqHGVKv48Y7HnkqnPnKFA2Q MAAEXxlChNZYBHmV7U05knsBgN/zsxH35GiYVMfQblXEeI/wIncFLqd8G8zSnUD8eq9w yOQg== X-Gm-Message-State: AIVw1125KIZyrVPtwEdDje7iYC8463bWT60+Qg9/pvAEbTwizWvKsAFq dfw9ZpR3SiS1AQ== X-Received: by 10.28.4.78 with SMTP id 75mr616041wme.66.1500488004185; Wed, 19 Jul 2017 11:13:24 -0700 (PDT) Received: from android-4c5a376a18c0e957.fritz.box (p5494E583.dip0.t-ipconnect.de. [84.148.229.131]) by smtp.gmail.com with ESMTPSA id d19sm1141427wrb.93.2017.07.19.11.13.22 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 19 Jul 2017 11:13:22 -0700 (PDT) Date: Wed, 19 Jul 2017 18:13:00 -0000 User-Agent: K-9 Mail for Android In-Reply-To: References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq To: Andrew Pinski CC: GCC Patches From: Richard Biener Message-ID: <6B04BC1D-13FA-4AED-90D1-6AD6BF10BDD2@gmail.com> X-IsSubscribed: yes X-SW-Source: 2017-07/txt/msg01183.txt.bz2 On July 19, 2017 6:10:28 PM GMT+02:00, Andrew Pinski wr= ote: >On Mon, Jul 17, 2017 at 3:02 AM, Richard Biener > wrote: >> On Thu, Jul 13, 2017 at 6:18 AM, Andrew Pinski >wrote: >>> On Wed, Jul 12, 2017 at 9:10 PM, Marc Glisse >wrote: >>>> On Wed, 12 Jul 2017, Andrew Pinski wrote: >>>> >>>>> Hi, >>>>> Unlike most other expressions, BIT_INSERT_EXPR has an implicit >>>>> operand of the precision/size of the second operand. This means >if we >>>>> have an integer constant for the second operand and that compares >to >>>>> the same constant value, vn_nary_op_eq would return that these two >>>>> expressions are the same. But in the case I was looking into the >>>>> integer constants had different types, one with 1 bit precision >and >>>>> the other with 2 bit precision which means the BIT_INSERT_EXPR >were >>>>> not equal at all. >>>>> >>>>> This patches the problem by checking to see if BIT_INSERT_EXPR's >>>>> operand 1's (second operand) type has different precision to >return >>>>> false. >>>>> >>>>> Is this the correct location or should we be checking for this >>>>> differently? If this is the correct location, is the patch ok? >>>>> Bootstrapped and tested on aarch64-linux-gnu with no regressions >(and >>>>> also tested with a few extra patches to expose BIT_INSERT_EXPR). >>>>> >>>>> Thanks, >>>>> Andrew Pinski >>>>> >>>>> ChangeLog: >>>>> * tree-ssa-sccvn.c (vn_nary_op_eq): Check BIT_INSERT_EXPR's >operand 1 >>>>> to see if the types precision matches. >>>> >>>> >>>> Hello, >>>> >>>> since BIT_INSERT_EXPR is implicitly an expression with 4 arguments, >it makes >>>> sense that we may need a few such special cases. But shouldn't the >hash >>>> function be in sync with the equality comparator? Does >operand_equal_p need >>>> the same? >>> >>> The hash function does not need to be exactly the same. The only >>> requirement there is if vn_nary_op_eq returns true then the hash has >>> to be the same. Now we could improve the hash by using the >precision >>> which will allow us not to compare as much in some cases. >>> >>> Yes operand_equal_p needs the same handling; I did not notice that >>> until you mention it.. >>> Right now it does: >>> case BIT_INSERT_EXPR: >>> return OP_SAME (0) && OP_SAME (1) && OP_SAME (2); >> >> Aww. The issue is that operand_equal_p treats INTEGER_CSTs of >different >> type/precision but the same value as equal. >> >> Revisiting that, while a good idea, shouldn't block a fix here. So >... >> >> Index: tree-ssa-sccvn.c >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> --- tree-ssa-sccvn.c (revision 250159) >> +++ tree-ssa-sccvn.c (working copy) >> @@ -2636,6 +2636,14 @@ vn_nary_op_eq (const_vn_nary_op_t const >> if (!expressions_equal_p (vno1->op[i], vno2->op[i])) >> return false; >> >> + /* BIT_INSERT_EXPR has an implict operand as the type precision >> + of op1. Need to check to make sure they are the same. */ >> + if (vno1->opcode =3D=3D BIT_INSERT_EXPR) >> + if (INTEGRAL_TYPE_P (TREE_TYPE (vno1->op[0])) >> + && TYPE_PRECISION (TREE_TYPE (vno1->op[1])) >> + !=3D TYPE_PRECISION (TREE_TYPE (vno2->op[1]))) >> + return false; >> + >> >> the case can be restricted to INTEGER_CST vno1->op[0] I think: >> >> if (vno1->opcode =3D=3D BIT_INSERT_EXPR >> && TREE_CODE (vno1->op[0]) =3D=3D INTEGER_CST >> && TYPE_PRECISION (.... >> >> and yes, operand_equal_p needs a similar fix. Can you re-post with >that added? > >Here is that with the changes you requested too. > >> Do you have a testcase? > >I don't have one which fails with the trunk. With lowering of >bit-fields accesses (which I hope to submit soon; just getting in the >required patches first), many testcases fail (bootstrap fails for the >same reason too). > >OK? Bootstrapped and tested on aarch64-linux-gnu with no regressions. In the fold-const.c hunk you need to verify arg1 op0 is an INTEGER_CST. Th= is is not necessary in sccvn because we passed operand_equal_p first. OK with that change. Richard. >Thanks, >Andrew > >ChangeLog: >* tree-ssa-sccvn.c (vn_nary_op_eq): Check BIT_INSERT_EXPR's operand 1 >to see if the types precision matches. >* fold-const.c (operand_equal_p): Likewise, > > >> >> Thanks, >> Richard. >> >>> Thanks, >>> Andrew Pinski >>> >>>> >>>> -- >>>> Marc Glisse