From patchwork Tue Jul 4 12:41:52 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jakub Jelinek X-Patchwork-Id: 783936 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 3x23YN4cCNz9sQl for ; Tue, 4 Jul 2017 22:42:15 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="c0o/omSD"; 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:date :from:to:cc:subject:message-id:reply-to:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=MX/rGd1+rI2RB/xpb DbCo+bnT5GUf0mu24bk/EppJhs2snR4Ab/9ixwp0dMQ25hvMzPjdFefAKgZEj+Dg YIILFQymks7naNFRYrDEgCfUFjI+Zw99orKlroFgUYWN9KMezC61P8OWv2TMXW67 17TXbf2R3otcpVDmrpnT8WbNuU= 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:date :from:to:cc:subject:message-id:reply-to:references:mime-version :content-type:in-reply-to; s=default; bh=5uPUsN4tG93M09qU8USOcpZ sMNo=; b=c0o/omSDPjB1Vn881do1F6+OA/RQLtqHjJDXAEBPgx/Amiv5/tjBRQ9 72ve/qxt0QBcROMVr4u8JPGGrXybmPquJSK5ZHS8MabFIqHF99lzg5bRSPh/Pri6 B/hfNFnwzDkoYBWZB0fSdq0v2lIKWWzRyzglaB6h0BA598Onazl4= Received: (qmail 84833 invoked by alias); 4 Jul 2017 12:42:02 -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 84595 invoked by uid 89); 4 Jul 2017 12:42:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-16.9 required=5.0 tests=BAYES_00, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 04 Jul 2017 12:41:59 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 940511555A; Tue, 4 Jul 2017 12:41:57 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 940511555A Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=jakub@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 940511555A Received: from tucnak.zalov.cz (ovpn-116-143.ams2.redhat.com [10.36.116.143]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C4FFD6BF8C; Tue, 4 Jul 2017 12:41:56 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.15.2/8.15.2) with ESMTP id v64CfrhO007571; Tue, 4 Jul 2017 14:41:54 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id v64Cfq3E007570; Tue, 4 Jul 2017 14:41:52 +0200 Date: Tue, 4 Jul 2017 14:41:52 +0200 From: Jakub Jelinek To: Richard Biener Cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Fix -fcompare-debug issues caused by recent VRP assert expr sorting changes (PR debug/81278) Message-ID: <20170704124152.GM2123@tucnak> Reply-To: Jakub Jelinek References: <20170704113247.GI2123@tucnak> <20170704115023.GL2123@tucnak> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.7.1 (2016-10-04) X-IsSubscribed: yes On Tue, Jul 04, 2017 at 02:00:13PM +0200, Richard Biener wrote: > > That was intentional. If a->e != NULL, then we know that b->e != NULL, > > because we have > > else if (a->e != NULL && b->e == NULL) > > return -1; > > earlier. Similarly, if a->e == NULL, then we know that b-> == NULL, because > > we have: > > if (a->e == NULL && b->e != NULL) > > return 1; > > earlier. > > Ah, ok. Twisty ;) I suppose jump threading will have eliminated > the extra test. In the first case maybe, I doubt it would do that after the iterative_hash_expr calls which are likely not pure. > > > Otherwise looks ok to me. I wonder if we should merge the two > > > sorting functions and change behavior with a global var or a > > > template parameter instead (to reduce source duplication). Does > > > > > > vec.qsort (function_template); > > > > > > work? > > > > Let me try that. Seems to work, so like this if it passes bootstrap/regtest? 2017-07-04 Jakub Jelinek PR debug/81278 * tree-vrp.c (compare_assert_loc): Turn into a function template with stable template parameter. Only test if a->e is NULL, !a->e == !b->e has been verified already. Use e == NULL or e != NULL instead of e or ! e tests. If stable is true, don't use iterative_hash_expr, on the other side allow a or b or both NULL and sort the NULLs last. (process_assert_insertions): Sort using compare_assert_loc instead of compare_assert_loc, later sort using compare_assert_loc before calling process_assert_insertions_for in a loop. Use break instead of continue once seen NULL pointer. Jakub --- gcc/tree-vrp.c.jj 2017-07-04 10:43:48.627706528 +0200 +++ gcc/tree-vrp.c 2017-07-04 14:37:06.823101453 +0200 @@ -6393,20 +6393,37 @@ process_assert_insertions_for (tree name gcc_unreachable (); } -/* Qsort helper for sorting assert locations. */ +/* Qsort helper for sorting assert locations. If stable is true, don't + use iterative_hash_expr because it can be unstable for -fcompare-debug, + on the other side some pointers might be NULL. */ +template static int compare_assert_loc (const void *pa, const void *pb) { assert_locus * const a = *(assert_locus * const *)pa; assert_locus * const b = *(assert_locus * const *)pb; - if (! a->e && b->e) + + /* If stable, some asserts might be optimized away already, sort + them last. */ + if (stable) + { + if (a == NULL) + return b != NULL; + else if (b == NULL) + return -1; + } + + if (a->e == NULL && b->e != NULL) return 1; - else if (a->e && ! b->e) + else if (a->e != NULL && b->e == NULL) return -1; + /* After the above checks, we know that (a->e == NULL) == (b->e == NULL), + no need to test both a->e and b->e. */ + /* Sort after destination index. */ - if (! a->e && ! b->e) + if (a->e == NULL) ; else if (a->e->dest->index > b->e->dest->index) return 1; @@ -6419,11 +6436,27 @@ compare_assert_loc (const void *pa, cons else if (a->comp_code < b->comp_code) return -1; + hashval_t ha, hb; + + /* E.g. if a->val is ADDR_EXPR of a VAR_DECL, iterative_hash_expr + uses DECL_UID of the VAR_DECL, so sorting might differ between + -g and -g0. When doing the removal of redundant assert exprs + and commonization to successors, this does not matter, but for + the final sort needs to be stable. */ + if (stable) + { + ha = 0; + hb = 0; + } + else + { + ha = iterative_hash_expr (a->expr, iterative_hash_expr (a->val, 0)); + hb = iterative_hash_expr (b->expr, iterative_hash_expr (b->val, 0)); + } + /* Break the tie using hashing and source/bb index. */ - hashval_t ha = iterative_hash_expr (a->expr, iterative_hash_expr (a->val, 0)); - hashval_t hb = iterative_hash_expr (b->expr, iterative_hash_expr (b->val, 0)); if (ha == hb) - return (a->e && b->e + return (a->e != NULL ? a->e->src->index - b->e->src->index : a->bb->index - b->bb->index); return ha - hb; @@ -6452,7 +6485,7 @@ process_assert_insertions (void) auto_vec asserts; for (; loc; loc = loc->next) asserts.safe_push (loc); - asserts.qsort (compare_assert_loc); + asserts.qsort (compare_assert_loc); /* Push down common asserts to successors and remove redundant ones. */ unsigned ecnt = 0; @@ -6506,11 +6539,14 @@ process_assert_insertions (void) } } + /* The asserts vector sorting above might be unstable for + -fcompare-debug, sort again to ensure a stable sort. */ + asserts.qsort (compare_assert_loc); for (unsigned j = 0; j < asserts.length (); ++j) { loc = asserts[j]; if (! loc) - continue; + break; update_edges_p |= process_assert_insertions_for (ssa_name (i), loc); num_asserts++; free (loc);