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 [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id C41A53892029 for ; Sat, 21 Nov 2020 06:13:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C41A53892029 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-595-Zbyt_kGBP_OjiF-4UwPtAg-1; Sat, 21 Nov 2020 01:13:34 -0500 X-MC-Unique: Zbyt_kGBP_OjiF-4UwPtAg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 142ED1800D41; Sat, 21 Nov 2020 06:13:33 +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 988FB60BD8; Sat, 21 Nov 2020 06:13:32 +0000 (UTC) Subject: Re: Incremental updating of SSA_NAMEs that are passed to b_c_p To: Ilya Leoshkevich , gcc@gcc.gnu.org Cc: Marc Glisse References: <4ce44dda24d28279156422c10d09d64a575f1f8c.camel@linux.ibm.com> From: Jeff Law Message-ID: Date: Fri, 20 Nov 2020 23:13:31 -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: <4ce44dda24d28279156422c10d09d64a575f1f8c.camel@linux.ibm.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 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.5 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_H3, 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@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 21 Nov 2020 06:13:40 -0000 On 10/27/20 12:34 PM, Ilya Leoshkevich wrote: > Hi, > > I'd like to revive the old discussion regarding the interaction of > jump threading and b_c_p causing the latter to incorrectly return 1 in > certain cases: > > https://gcc.gnu.org/pipermail/gcc-patches/2020-June/547236.html > https://gcc.gnu.org/pipermail/gcc-patches/2020-July/549288.html > > The conclusion was that this happening during threading is just a > symptom of a deeper problem: SSA_NAMEs that are passed to b_c_p should > not be registered for incremental updating. That's perhaps not as precise as we'd like to be. Simply registering the SSA_NAME isn't the problem.  It's the need to generate a PHI to resolve the incremental update that causes the problems.    And as much as anything it's a sanity check that we're not filtering out jump threading cases that we should (ignore the case for now where the PHI we generate is a degenerate). Now it may be painful to actually detect those cases.  I haven't looked at the into-ssa bits in years. > > I performed a little experiment and added an assertion to > create_new_def_for: > > --- a/gcc/tree-into-ssa.c > +++ b/gcc/tree-into-ssa.c > @@ -2996,6 +3014,8 @@ create_new_def_for (tree old_name, gimple *stmt, > def_operand_p def) > { > tree new_name; > > + gcc_checking_assert (!used_by_bcp_p (old_name)); > + > timevar_push (TV_TREE_SSA_INCREMENTAL); > > if (!update_ssa_initialized_fn) > > This has of course fired when performing basic block duplication during > threading, which can be fixed by avoiding duplication of basic blocks > wi > th b_c_p calls: > > --- a/gcc/tree-cfg.c > +++ b/gcc/tree-cfg.c > @@ -6224,7 +6224,8 @@ gimple_can_duplicate_bb_p (const_basic_block bb) > || gimple_call_internal_p (g, IFN_GOMP_SIMT_EXIT) > || gimple_call_internal_p (g, IFN_GOMP_SIMT_VOTE_ANY) > || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_BFLY) > - || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_IDX))) > + || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_IDX) > + || gimple_call_builtin_p (g, BUILT_IN_CONSTANT_P))) > return false; > } Yea, that's a reasonable place to start with the checking assert to see how pervasive these problems are.  I probably wouldn't just blindly disable threading through blocks with b_c_p call, but again, it seems like a reasonable thing to do to help get a sense of other problem areas. > > The second occurrence is a bit more interesting: > > gimple * > vrp_insert::build_assert_expr_for (tree cond, tree v) > { > ... > a = build2 (ASSERT_EXPR, TREE_TYPE (v), v, cond); > assertion = gimple_build_assign (NULL_TREE, a); > ... > tree new_def = create_new_def_for (v, assertion, NULL); > > The fix is also simple though: > > --- a/gcc/tree-vrp.c > +++ b/gcc/tree-vrp.c > @@ -3101,6 +3101,9 @@ vrp_insert::process_assert_insertions_for (tree > name, assert_locus *loc) > if (loc->expr == loc->val) > return false; > > + if (used_by_bcp_p (name)) > + return false; > + > cond = build2 (loc->comp_code, boolean_type_node, loc->expr, loc- >> val); > assert_stmt = build_assert_expr_for (cond, name); > if (loc->e) > > My original testcase did not trigger anything else. I'm planning to > check how this affects the testsuite, but before going further I would > like to ask: is this the right direction now? To me it looks a > little-bit more heavy-handed than the original approach, but maybe it's > worth it. It seems  a bit heavy-handed.  But that's not based on any hard data, just a sense that we may not want to disable threading this aggressively. jeff