From patchwork Fri Aug 5 13:58:03 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Biener X-Patchwork-Id: 1664063 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: bilbo.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.a=rsa-sha256 header.s=default header.b=pI9kMpfT; dkim-atps=neutral Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=sourceware.org; envelope-from=gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Received: from sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by bilbo.ozlabs.org (Postfix) with ESMTPS id 4LznHq0rb4z9s2R for ; Fri, 5 Aug 2022 23:58:31 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 0330E385800E for ; Fri, 5 Aug 2022 13:58:26 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0330E385800E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1659707906; bh=DEPSl/NdgP77Hj90bn17UARmYLpb7UCaJ6GPmiAMAxw=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=pI9kMpfTLM28TU39cwMxZoaDmZhfzyjkPNCDDJZYaVQszvweqBFURhqIjGgaKodeS mmhSCtLRsyBhij4Eh6i0yK6TB0O4EnpF4U/HKwlWGS/ATVezxstFbg2/Hsj5fwDItl ueaUqrIroQEGl9VfEB2tQG3smPP4bh0SoHs7UQSo= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by sourceware.org (Postfix) with ESMTPS id A026C3858C53 for ; Fri, 5 Aug 2022 13:58:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A026C3858C53 Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id AA81F3427C; Fri, 5 Aug 2022 13:58:03 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 8DAE2133B5; Fri, 5 Aug 2022 13:58:03 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id YkNEIesh7WLNPAAAMHmgww (envelope-from ); Fri, 05 Aug 2022 13:58:03 +0000 Date: Fri, 5 Aug 2022 15:58:03 +0200 (CEST) To: gcc-patches@gcc.gnu.org Subject: [PATCH] Adjust backwards threader costing of PHIs MIME-Version: 1.0 Message-Id: <20220805135803.8DAE2133B5@imap2.suse-dmz.suse.de> X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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-Patchwork-Original-From: Richard Biener via Gcc-patches From: Richard Biener Reply-To: Richard Biener Errors-To: gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org Sender: "Gcc-patches" The following adjusts the costing of PHIs to match how I understand the comment and maybe the original intent. The will be no non-degenerate PHI nodes remaining on the threaded path but when there are alternate path exits PHI nodes at their destinations will likely require extra copies on those edges and that's what we want to account for. Jeff added this code long time ago and at least the special-casing of (just) m_name does not make sense anymore. Unfortunately this tweaks heuristics enough to make the threader thread an unfortunate path in libgomp/team.c so that -Walloca-larger-than diagnoses an allocation of -1 elements. I'm not 100% sure this condition is impossible so I've added a guard not allocating zero or "less" stack. There's also an uninit diagnostic in opts.cc about best_math::m_best_candidate_len that looks like a false positive but all but this member are initialized so the patch initializes this one as well to avoid this false positive. I have yet to analyze some fallout as well: FAIL: gcc.dg/uninit-pred-9_b.c bogus warning (test for bogus messages, line 20) FAIL: gcc.dg/tree-ssa/phi_on_compare-4.c scan-tree-dump-times dom2 "Removing bas ic block" 1 FAIL: gcc.dg/tree-ssa/pr59597.c scan-tree-dump-times threadfull1 "Registering jump thread" 4 FAIL: gcc.dg/tree-ssa/pr61839_2.c scan-tree-dump-times evrp "%" 0 FAIL: gcc.dg/tree-ssa/pr61839_2.c scan-tree-dump-times evrp "972195717 % " 0 FAIL: gcc.dg/tree-ssa/pr61839_2.c scan-tree-dump-times evrp "972195717 / " 0 FAIL: gcc.dg/tree-ssa/pr61839_2.c scan-tree-dump-times evrp "__builtin_abort" 1 the early threading fails are because we now account for the mid-exit PHI copies and early threading has a limit of a single copied insn. But this testcase was never about threading but VRP which seemingly regressed meanwhile... Bootstrapped and tested (with the above FAILs) on x86_64-unknown-linux-gnu. Any comments besides the FAILout? Thanks, Richard. gcc/ * tree-ssa-threadbackward.cc (back_threader_profitability::profitable_path_p): Rewrite the costing of PHIs to instead reflect the cost of alternate path exits. Remove m_name argument. (back_threader::find_paths_to_names): Adjust. (back_threader::maybe_register_path): Likewise. * spellcheck.h (best_math::m_best_candidate_len): Initialize. libgomp/ * team.c (gomp_team_start): Guard alloca. --- gcc/spellcheck.h | 3 +- gcc/tree-ssa-threadbackward.cc | 65 ++++++++++++---------------------- libgomp/team.c | 5 +-- 3 files changed, 28 insertions(+), 45 deletions(-) diff --git a/gcc/spellcheck.h b/gcc/spellcheck.h index 3706de38a9d..1b76a54b678 100644 --- a/gcc/spellcheck.h +++ b/gcc/spellcheck.h @@ -95,7 +95,8 @@ class best_match : m_goal (goal_traits::get_string (goal)), m_goal_len (goal_traits::get_length (goal)), m_best_candidate (NULL), - m_best_distance (best_distance_so_far) + m_best_distance (best_distance_so_far), + m_best_candidate_len (0) {} /* Compare the edit distance between CANDIDATE and m_goal, diff --git a/gcc/tree-ssa-threadbackward.cc b/gcc/tree-ssa-threadbackward.cc index 332a1d2a1dd..3377e648445 100644 --- a/gcc/tree-ssa-threadbackward.cc +++ b/gcc/tree-ssa-threadbackward.cc @@ -64,7 +64,7 @@ public: back_threader_profitability (bool speed_p) : m_speed_p (speed_p) { } - bool profitable_path_p (const vec &, tree name, edge taken, + bool profitable_path_p (const vec &, edge taken, bool *irreducible_loop = NULL); private: const bool m_speed_p; @@ -241,7 +241,7 @@ back_threader::maybe_register_path () else { bool irreducible = false; - if (m_profit.profitable_path_p (m_path, m_name, taken_edge, + if (m_profit.profitable_path_p (m_path, taken_edge, &irreducible) && debug_counter () && m_registry.register_path (m_path, taken_edge)) @@ -348,7 +348,7 @@ back_threader::find_paths_to_names (basic_block bb, bitmap interesting) // Try to resolve the path without looking back. if (m_path.length () > 1 - && (!m_profit.profitable_path_p (m_path, m_name, NULL) + && (!m_profit.profitable_path_p (m_path, NULL) || maybe_register_path ())) ; @@ -529,9 +529,6 @@ back_threader::debug () /* Examine jump threading path PATH and return TRUE if it is profitable to thread it, otherwise return FALSE. - NAME is the SSA_NAME of the variable we found to have a constant - value on PATH. If unknown, SSA_NAME is NULL. - If the taken edge out of the path is known ahead of time it is passed in TAKEN_EDGE, otherwise it is NULL. @@ -543,7 +540,6 @@ back_threader::debug () bool back_threader_profitability::profitable_path_p (const vec &m_path, - tree name, edge taken_edge, bool *creates_irreducible_loop) { @@ -600,42 +596,27 @@ back_threader_profitability::profitable_path_p (const vec &m_path, if (j < m_path.length () - 1) { int orig_n_insns = n_insns; - /* PHIs in the path will create degenerate PHIS in the - copied path which will then get propagated away, so - looking at just the duplicate path the PHIs would - seem unimportant. - - But those PHIs, because they're assignments to objects - typically with lives that exist outside the thread path, - will tend to generate PHIs (or at least new PHI arguments) - at points where we leave the thread path and rejoin - the original blocks. So we do want to account for them. - - We ignore virtual PHIs. We also ignore cases where BB - has a single incoming edge. That's the most common - degenerate PHI we'll see here. Finally we ignore PHIs - that are associated with the value we're tracking as - that object likely dies. */ - if (EDGE_COUNT (bb->succs) > 1 && EDGE_COUNT (bb->preds) > 1) + /* If the path has exits other than the known taken_edge from + block j == 0 then those will create new edges into the exit + destination, increasing the number of PHI arguments there. + Account for an extra copy there. + PHI nodes on the path itself all become degenerate and + propagate out on GIMPLE. */ + if (j != 0 && EDGE_COUNT (bb->succs) > 1) { - for (gphi_iterator gsip = gsi_start_phis (bb); - !gsi_end_p (gsip); - gsi_next (&gsip)) - { - gphi *phi = gsip.phi (); - tree dst = gimple_phi_result (phi); - - /* Note that if both NAME and DST are anonymous - SSA_NAMEs, then we do not have enough information - to consider them associated. */ - if (dst != name - && name - && TREE_CODE (name) == SSA_NAME - && (SSA_NAME_VAR (dst) != SSA_NAME_VAR (name) - || !SSA_NAME_VAR (dst)) - && !virtual_operand_p (dst)) - ++n_insns; - } + edge_iterator ei; + edge e; + FOR_EACH_EDGE (e, ei, bb->succs) + if (e->dest != m_path[j - 1]) + for (gphi_iterator gsip = gsi_start_phis (e->dest); + !gsi_end_p (gsip); + gsi_next (&gsip)) + { + gphi *phi = gsip.phi (); + tree arg = PHI_ARG_DEF_FROM_EDGE (phi, e); + if (!virtual_operand_p (arg)) + ++n_insns; + } } if (!contains_hot_bb && m_speed_p) diff --git a/libgomp/team.c b/libgomp/team.c index cb6875d70fa..89b263ef148 100644 --- a/libgomp/team.c +++ b/libgomp/team.c @@ -756,8 +756,9 @@ gomp_team_start (void (*fn) (void *), void *data, unsigned nthreads, attr = &thread_attr; } - start_data = gomp_alloca (sizeof (struct gomp_thread_start_data) - * (nthreads - i)); + if (i < nthreads) + start_data = gomp_alloca (sizeof (struct gomp_thread_start_data) + * (nthreads - i)); /* Launch new threads. */ for (; i < nthreads; ++i)