From patchwork Mon Jul 17 10:02:58 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Biener X-Patchwork-Id: 789353 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 3x9zPt3VMtz9sxR for ; Mon, 17 Jul 2017 20:03:14 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="EfO3Jo5n"; 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:from:date:message-id :subject:to:cc:content-type; q=dns; s=default; b=Jq0zI0YsaRe1E1x ff8TvCkQOZRK1WFT3hblO59dSHWTQghKLYw1oXlGI97ARP1/6x1QvGO9dHVCNOQ2 MI+KuGC9lK+cl7oUK00WlrohNDUCNnY62BUviYF3MjXAYj8bSO4fEqcUE3Z5cCEJ xMjxXer76JSSOqUCziycLioDy5mg= 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:from:date:message-id :subject:to:cc:content-type; s=default; bh=xUsI5OlWFHvB6hX4UH9w0 ZdNMuU=; b=EfO3Jo5napN5MzXz1EkyYDqg2/MNm3AdS6hQYAXhCBI6kb0mqdJuf YsWzXdj1ZV32oOWfe/wFEtUDyHj679cbOvoEuo9glWWF957twT4DFBTYT3GSXeHN bqLUeSnLd6oPmFlGV/DTRVC5hm8pefKzCpNRaWQRmMUMyKwIb93UwI= Received: (qmail 53576 invoked by alias); 17 Jul 2017 10:03:04 -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 53470 invoked by uid 89); 17 Jul 2017 10:03:02 -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-lf0-f44.google.com Received: from mail-lf0-f44.google.com (HELO mail-lf0-f44.google.com) (209.85.215.44) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 17 Jul 2017 10:03:01 +0000 Received: by mail-lf0-f44.google.com with SMTP id h22so82234683lfk.3 for ; Mon, 17 Jul 2017 03:03:00 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Xn2F4ETGHJ/c39c2Lhxgxx+upaX5ve07zWXxR6egMsE=; b=PWqqLBa5sC2k2KSeF+QqixjmHFpKO7Cvi3pPZFsciMDrekpqBmq2Hl9mgShLdgagdC rKevIfBATBYVqQWTiVQMYfXnYDtEk7Hs9MVmSKBd3/dlxaj4anUCJPu7MyTQY5GDnJSm ZvKboIlqtlGOTt7NsYMDv0bKRZU43E9fttlcMDMweabiRti9rFoCC3CubISfV6TCVmFX wDReAxTsxiVwBQP5EgEKZb1AhtLn23XSegmSTNH7Q+jF4LTPOpdcs4DIlpz6kZnoBEGt xsYRpwS5G/Rnx7rLxEheRQsMQJC0kogQ29miS7F1pv2EFHwta32ILB7lu2q2kdjjb4L0 VCsw== X-Gm-Message-State: AIVw1116NWhpU3XRgJ2ccKcSOC7jT9+teJFdYKzvg5VyikxKZm68xL4k WdjGKocNFJgH1OIyiWsjs1C4qcJa3w== X-Received: by 10.46.9.71 with SMTP id 68mr4511591ljj.120.1500285779026; Mon, 17 Jul 2017 03:02:59 -0700 (PDT) MIME-Version: 1.0 Received: by 10.25.31.134 with HTTP; Mon, 17 Jul 2017 03:02:58 -0700 (PDT) In-Reply-To: References: From: Richard Biener Date: Mon, 17 Jul 2017 12:02:58 +0200 Message-ID: Subject: Re: [RFC/SCCVN] Handle BIT_INSERT_EXPR in vn_nary_op_eq To: Andrew Pinski Cc: GCC Patches X-IsSubscribed: yes 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 ... if (vno1->opcode == BIT_INSERT_EXPR && TREE_CODE (vno1->op[0]) == INTEGER_CST && TYPE_PRECISION (.... and yes, operand_equal_p needs a similar fix. Can you re-post with that added? Do you have a testcase? Thanks, Richard. > Thanks, > Andrew Pinski > >> >> -- >> Marc Glisse Index: tree-ssa-sccvn.c =================================================================== --- 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 == BIT_INSERT_EXPR) + if (INTEGRAL_TYPE_P (TREE_TYPE (vno1->op[0])) + && TYPE_PRECISION (TREE_TYPE (vno1->op[1])) + != TYPE_PRECISION (TREE_TYPE (vno2->op[1]))) + return false; + the case can be restricted to INTEGER_CST vno1->op[0] I think: