From patchwork Wed Jan 29 02:54:51 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Law X-Patchwork-Id: 1230701 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-518460-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.a=rsa-sha1 header.s=default header.b=p0H4MiNE; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=GFOwvRo4; dkim-atps=neutral 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 486p4Y2qfXz9sNF for ; Wed, 29 Jan 2020 13:55:07 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:subject:from:reply-to:to:date:mime-version :content-type; q=dns; s=default; b=qfL2r9SCEL+MGQDOQvlGKoW8I0xKN pi25vnfdEkkTAiyUIiuTVQMelAzdBQSfwYOg3AtE8kM/wt7lyO6hroS2AJ61MgoU g5nbC/rDgiheb+gJkAoUA8/EbkNxDyYSFnGcQh3CZqox8QE5ZgUOA8PjRBhuTjxP cv5lm0LfBJ+i6Y= 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 :message-id:subject:from:reply-to:to:date:mime-version :content-type; s=default; bh=eU22WmnC+Z+YTCd0Tt2L8LWOfWg=; b=p0H 4MiNE7b2GOIYMnkCPOyatCJ5LrJccm8FIs0TRpzZDwFnIWRJpUg1UjlfbKwZ6Ldz jDCIHJRBAl2p1touByMUPF4DMc7gmBVKgNZEx35s9Dq3zSy8EzGVK3WNZ5U3OEyA HLQBeJC/aleZ/UJcOe7wrITfgI+L/FGwjygQB/nQ= Received: (qmail 24768 invoked by alias); 29 Jan 2020 02:54:59 -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 24759 invoked by uid 89); 29 Jan 2020 02:54:59 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.7 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=degree X-HELO: us-smtp-delivery-1.mimecast.com Received: from us-smtp-2.mimecast.com (HELO us-smtp-delivery-1.mimecast.com) (205.139.110.61) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 29 Jan 2020 02:54:57 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1580266495; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:mime-version:mime-version: content-type:content-type; bh=2dG+h46TdXfw6QkxF95PmkGb0dKJCFa8pOCG5dl3sh8=; b=GFOwvRo4j4UJl447JNK5AsuuOKtfftUTsrY11s/3YlyM7zBRgj+TrW+XcB6yTm7P3zvSrs rJgZ7fMAungRvmowPolYSbyQoYrQeqRoZiIKoLE5/toSLecDrJVkQlbPHLiWfmKVOOsI3T 4WQL9t/3jS3fL2O47XU55TNRiMdDs5w= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-390-MzNqMxU4MqKUunvu9eBcJQ-1; Tue, 28 Jan 2020 21:54:53 -0500 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 765AB107ACCD for ; Wed, 29 Jan 2020 02:54:52 +0000 (UTC) Received: from ovpn-117-120.phx2.redhat.com (ovpn-117-120.phx2.redhat.com [10.3.117.120]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4687880894 for ; Wed, 29 Jan 2020 02:54:52 +0000 (UTC) Message-ID: <340e993bb94e3f8d42b3eaaa4fda63851db73638.camel@redhat.com> Subject: [RFA/RFC] Improve DSE/aliasing around __builtin_object_size (PR89689, P2 regression) From: Jeff Law Reply-To: law@redhat.com To: gcc-patches List Date: Tue, 28 Jan 2020 19:54:51 -0700 User-Agent: Evolution 3.34.3 (3.34.3-1.fc31) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-IsSubscribed: yes Here's two approaches to fix the regression in 89689. Note this only fixes the regression in the originally reported testcase. THe deeper issue Martin raises in C#1 is not addressed. Thus if we go forward with either patch ISTM that we would drop the regression markers, but keep the BZ open. So the key blocks in question as we enter DSE1: > > ;; basic block 2, loop depth 0, maybe hot > ;; prev block 0, next block 3, flags: (NEW, REACHABLE, VISITED) > ;; pred: ENTRY (FALLTHRU,EXECUTABLE) > h = l; > h$data_33 = l.data; > if (h$data_33 != &__sb_slop) > goto ; [70.00%] > else > goto ; [30.00%] > ;; succ: 3 [70.0% (guessed)] (TRUE_VALUE,EXECUTABLE) > ;; 4 [30.0% (guessed)] (FALSE_VALUE,EXECUTABLE) > > ;; basic block 3, loop depth 0, maybe hot > ;; prev block 2, next block 4, flags: (NEW, REACHABLE, VISITED) > ;; pred: 2 [70.0% (guessed)] (TRUE_VALUE,EXECUTABLE) > *h$data_33 = 0; > ;; succ: 4 [always] (FALLTHRU,EXECUTABLE) > > ;; basic block 4, loop depth 0, maybe hot > ;; prev block 3, next block 5, flags: (NEW, REACHABLE, VISITED) > ;; pred: 3 [always] (FALLTHRU,EXECUTABLE) > ;; 2 [30.0% (guessed)] (FALSE_VALUE,EXECUTABLE) > _23 = __builtin_object_size (h$data_33, 0); > _25 = __builtin___memcpy_chk (h$data_33, "abcd", 4, _23); > __builtin_puts (h$data_33); > _6 = h$data_33 == &__sb_slop; > _7 = (int) _6; > printf ("%d\n", _7); > _9 = h$data_33 == &buf; > _10 = (int) _9; > printf ("%d\n", _10); > if (h$data_33 != &__sb_slop) > goto ; [70.00%] > else > goto ; [30.00%] > So vrp1 is going to want to thread the jump at the end of bb4 which requires duplicating bb4. One version of bb4 will only be reachable via the false edge from bb2. That in turn exposes a cprop opportunity to replace h$data_33 with &__sbloop and the type of &__sbloop is a char[1] array thus triggering the false positive warning. We can avoid all that by realizing the store in bb3 is dead. That in turn makes the conditional at the end of bb2 pointless as bb3 is empty and thus both arms ultimately transfer control to bb4 without any other side effects meaning we can eliminate that conditional which in turn eliminates the need for jump threading. Again, this only fixes the original testcase and if there were other statements in bb3 it wouldn't work and it won't work for the test in c#1. But the proposed changes are a general improvement. DSE misses this case because ref_maybe_used_by_stmt_p returns true for the virtual operand of the __builtin_object_size call. Opps! There's two easy ways to fix this, I've bootstrapped and regression tested both. First, the most conservative fix and perhaps the most appropriate for gcc-10 -- special casing __builtin_object_size in tree-ssa-dse.c. The second approach is more general and marks __builtin_object_size as const rather than just pure. Jakub and I kicked this around in IRC a bit. We've always marked __builtin_object_size as pure. It was never const. THere is some concern that code motion might cause _b_o_s to get different results, which would be particularly concerning for _b_o_s types #1 and #3 which do use some contextual information. However, ISTM that motion is still going to be limited by the SSA graph, though somewhat less because we wouldn't have a dependency on the virtual operands. So it *should* be safe, but there's some degree of hesitation to make this change at this point in gcc-10's lifecycle. Thoughts? If we go forward obviously I'd add a testcase and ChangeLog entries. Jeff commit f4e5d3f4755b6a5846ac20b53008b90131a8bb7c Author: Jeff Law Date: Tue Jan 28 18:16:09 2020 -0500 Mark _b_o_s as constant diff --git a/gcc/builtins.def b/gcc/builtins.def index 5ab842c34c2..fa8b0641ab1 100644 --- a/gcc/builtins.def +++ b/gcc/builtins.def @@ -972,7 +972,7 @@ DEF_BUILTIN_STUB (BUILT_IN_STRCMP_EQ, "__builtin_strcmp_eq") DEF_BUILTIN_STUB (BUILT_IN_STRNCMP_EQ, "__builtin_strncmp_eq") /* Object size checking builtins. */ -DEF_GCC_BUILTIN (BUILT_IN_OBJECT_SIZE, "object_size", BT_FN_SIZE_CONST_PTR_INT, ATTR_PURE_NOTHROW_LEAF_LIST) +DEF_GCC_BUILTIN (BUILT_IN_OBJECT_SIZE, "object_size", BT_FN_SIZE_CONST_PTR_INT, ATTR_CONST_NOTHROW_LEAF_LIST) DEF_EXT_LIB_BUILTIN (BUILT_IN_MEMCPY_CHK, "__memcpy_chk", BT_FN_PTR_PTR_CONST_PTR_SIZE_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) DEF_EXT_LIB_BUILTIN (BUILT_IN_MEMMOVE_CHK, "__memmove_chk", BT_FN_PTR_PTR_CONST_PTR_SIZE_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF) DEF_EXT_LIB_BUILTIN (BUILT_IN_MEMPCPY_CHK, "__mempcpy_chk", BT_FN_PTR_PTR_CONST_PTR_SIZE_SIZE, ATTR_RETNONNULL_NOTHROW_LEAF) diff --git a/gcc/tree-ssa-dse.c b/gcc/tree-ssa-dse.c index 374143e5785..e88342f1b68 100644 --- a/gcc/tree-ssa-dse.c +++ b/gcc/tree-ssa-dse.c @@ -789,8 +789,13 @@ dse_classify_store (ao_ref *ref, gimple *stmt, phi_def = use_stmt; } } - /* If the statement is a use the store is not dead. */ - else if (ref_maybe_used_by_stmt_p (use_stmt, ref)) + /* If the statement is a use the store is not dead. Do not + consider calls to __builtin_object_size as a use. For gcc-11 + we plan mark __builtin_object_size as const. */ + else if ((!is_gimple_call (use_stmt) + || !gimple_call_builtin_p (as_a (use_stmt), + BUILT_IN_OBJECT_SIZE)) + && ref_maybe_used_by_stmt_p (use_stmt, ref)) { /* Handle common cases where we can easily build an ao_ref structure for USE_STMT and in doing so we find that the