From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by sourceware.org (Postfix) with ESMTP id 1CBC4396EC7C for ; Fri, 20 Nov 2020 19:14:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 1CBC4396EC7C 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-48-aRKfBdXSPpW3WLOM1TfyuQ-1; Fri, 20 Nov 2020 14:14:49 -0500 X-MC-Unique: aRKfBdXSPpW3WLOM1TfyuQ-1 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 E0CD01005D64; Fri, 20 Nov 2020 19:14:47 +0000 (UTC) Received: from localhost.localdomain (ovpn-113-152.phx2.redhat.com [10.3.113.152]) by smtp.corp.redhat.com (Postfix) with ESMTP id 18A115D6AD; Fri, 20 Nov 2020 19:14:47 +0000 (UTC) Subject: Re: [PATCH v2] tree-ssa-threadbackward.c (profitable_jump_thread_path): Do not allow __builtin_constant_p () before IPA. To: Ilya Leoshkevich , gcc-patches@gcc.gnu.org Cc: Marc Glisse , Richard Biener , Jakub Jelinek References: <20200630184602.1228644-1-iii@linux.ibm.com> From: Jeff Law Message-ID: <4cee94a0-2739-4e98-f8e5-ca16747a5b51@redhat.com> Date: Fri, 20 Nov 2020 12:14:46 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.3.1 MIME-Version: 1.0 In-Reply-To: <20200630184602.1228644-1-iii@linux.ibm.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Content-Language: en-US X-Spam-Status: No, score=-5.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 20 Nov 2020 19:14:55 -0000 On 6/30/20 12:46 PM, Ilya Leoshkevich wrote: > v1: https://gcc.gnu.org/pipermail/gcc-patches/2020-June/547236.html > > This is the implementation of Jakub's suggestion: allow > __builtin_constant_p () after IPA, but fold it into 0. Smoke test > passed on s390x-redhat-linux, full regtest and bootstrap are running on > x86_64-redhat-linux. > > --- > > Linux Kernel (specifically, drivers/leds/trigger/ledtrig-cpu.c) build > with GCC 10 fails on s390 with "impossible constraint". > > The problem is that jump threading makes __builtin_constant_p () lie > when it splits a path containing a non-constant expression in a way > that on each of the resulting paths this expression is constant. > > Fix by disallowing __builtin_constant_p () on threading paths before > IPA and fold it into 0 after IPA. > > gcc/ChangeLog: > > 2020-06-30 Ilya Leoshkevich > > * tree-ssa-threadbackward.c (thread_jumps::m_allow_bcp_p): New > member. > (thread_jumps::profitable_jump_thread_path): Do not allow > __builtin_constant_p () on threading paths unless m_allow_bcp_p > is set. > (thread_jumps::find_jump_threads_backwards): Set m_allow_bcp_p. > (pass_thread_jumps::execute): Allow __builtin_constant_p () on > threading paths after IPA. > (pass_early_thread_jumps::execute): Do not allow > __builtin_constant_p () on threading paths before IPA. > * tree-ssa-threadupdate.c (duplicate_thread_path): Fold > __builtin_constant_p () on threading paths into 0. > > gcc/testsuite/ChangeLog: > > 2020-06-30 Ilya Leoshkevich > > * gcc.target/s390/builtin-constant-p-threading.c: New test. So I'm finally getting back to this.  Thanks for your patience. It's a nasty little problem, and I suspect there's actually some deeper issues here.  While I'd like to claim its a bad use of b_c_p, I don't think I can reasonably make that argument. So what we have is a b_c_p at the start of an if-else chain.  Subsequent tests on the "true" arm of the the b_c_p test may throw us off the constant path (because the constants are out of range).  Once all the tests are passed (it's constant and the constant is in range) the true arm's terminal block has a special asm that requires a constant argument.   In the case where we get to the terminal block on the true arm, the argument to the b_c_p is used as the constant argument to the special asm. At first glace jump threading seems to be doing the right thing.  Except that we end up with two paths to that terminal block with the special asm, one for each of the two constant arguments to the b_c_p call.  Naturally since that same value is used in the asm, we have to introduce a PHI to select between them at the head of the terminal block.   Now the argument in the asm is no longer constant and boom we fail. I briefly pondered if we should only throttle when the argument to the b_c_p is not used elsewhere.  But I think that just hides the problem and with a little work I could probably extend the testcase to still fail in that scenario. I also briefly pondered if we should isolate the terminal block as well (essentially creating one for each unique PHI argument).  We'd likely only need to do that when there's an ASM in the terminal block, but that likely just papers over the problem as well since the ASM could be in a successor of the terminal block. I haven't thought real deeply about it, but I wouldn't be surprised if there's other passes that can trigger similar problems.  Aggressive cross-jumping would be the most obvious, but some of the hosting/sinking of operations past PHIs would seem potentially problematical as well. Jakub suggestion might be the best one in this space.   I don't have anything better right now.  The deeper questions about other passes setting up similar scenarios can probably be punted, I'd expect threading to be far and above the most common way for this to happen and I'd be comfortable faulting in investigation of other cases if/when they happen. So I retract my initial objections.  Let's go with the V2 patch. jeff