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 8950E3857C78 for ; Tue, 1 Dec 2020 22:34:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8950E3857C78 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-198-2swO8ZbsP4mMnJHlLdKonw-1; Tue, 01 Dec 2020 17:34:44 -0500 X-MC-Unique: 2swO8ZbsP4mMnJHlLdKonw-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D871F3E74A; Tue, 1 Dec 2020 22:34:42 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-145.phx2.redhat.com [10.3.112.145]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0341210016FE; Tue, 1 Dec 2020 22:34:41 +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> <4cee94a0-2739-4e98-f8e5-ca16747a5b51@redhat.com> From: Jeff Law Message-ID: <8371e7e5-5e6d-561d-0a95-c67de6af4e96@redhat.com> Date: Tue, 1 Dec 2020 15:34:41 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 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=-6.2 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: Tue, 01 Dec 2020 22:34:48 -0000 On 11/23/20 7:36 AM, Ilya Leoshkevich wrote: > On Fri, 2020-11-20 at 12:14 -0700, Jeff Law wrote: >> 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 > Hi Jeff, > > Thanks for having another look! > > I did x86_64 builds of SPEC and vmlinux, and it seems that in practice > v2 does not have any benefit over v1. > > What do you think about going with the v1, which is less complex? No strong opinions.  I think whichever is less invasive in terms of code quality is probably the way to go.  What we want to avoid is suppressing threading unnecessarily as that often leads to false positives from middle-end based warnings.  Suppressing threading can also lead to build failures in the kernel due to the way they use b_c_p. jeff