From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id 096063858D32 for ; Mon, 29 Aug 2022 12:06:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 096063858D32 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de 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-out2.suse.de (Postfix) with ESMTPS id 0A6971F461 for ; Mon, 29 Aug 2022 12:06:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1661774761; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type; bh=y2EidYhRUmCz2QXf5da9mIE2zS41AwRDISIzkSmw15o=; b=EqkHutePUAEN1I5Q+wDjvt1qxZp+HE3j97FlFAU6SIgpWtJP+dvLIxe0Pa0asmGmb24cyI NyXUggC5HUiH8UMSjBYBNl3PGpFgpzhTYDzthMObT/KV+1tjrw5lJvYzfZotVMlsyHpPKc /ji96sxb0EGZgVZekDynZzdU0gP8zqg= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1661774761; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type; bh=y2EidYhRUmCz2QXf5da9mIE2zS41AwRDISIzkSmw15o=; b=8d1C02nCWdXe2hdNViMNq7U9d/IM6rKdJ2441cnM7KAsswQf/zOIbtuspGl04m3tf4QseM Pg49vayHYWhh7yAw== 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 E9E4A133A6 for ; Mon, 29 Aug 2022 12:06:00 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id f5DoN6irDGMRLAAAMHmgww (envelope-from ) for ; Mon, 29 Aug 2022 12:06:00 +0000 Date: Mon, 29 Aug 2022 14:06:00 +0200 (CEST) From: Richard Biener To: gcc-patches@gcc.gnu.org Subject: [PATCH] Make uninit PHI processing more consistent MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Message-Id: <20220829120600.E9E4A133A6@imap2.suse-dmz.suse.de> X-Spam-Status: No, score=-11.8 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 List-Id: Currently the main working of the maybe-uninit pass is to scan over all PHIs with possibly undefined arguments, diagnosing whether there's a direct not guarded use. For not guarded uses in PHIs those are queued for later processing and to make the uninit analysis PHI def handling work, mark the PHI def as possibly uninitialized. But this happens only for those PHI uses that happen to be seen before a direct not guarded use and whether all arguments of a PHI node which are defined by a PHI are properly marked as maybe uninitialized depends on the processing order. The following changes the uninit pass to perform an RPO walk over the function, ensuring that PHI argument defs are visited before the PHI node (besides backedge uses which we ignore already), getting rid of the worklist. It also makes sure to process all PHI uses, but recording those that are properly guarded so they are not treated as maybe undefined when processing the PHI use later. Overall this should make behavior more consistent, avoid some false negative because of the previous early out and order issue, and avoid some false positive because of the missed recording of guarded PHI uses. The patch correctly diagnoses an uninitalized use of 'regnum' in store_bit_field_1 but also diagnoses an uninitialized use of best_match::m_best_candidate_len which I've chosen to silence without analyzing it in detail (I'm doing that right now). Bootstrapped and tested on x86_64-unknown-linux-gnu. * gimple-predicate-analysis.h (uninit_analysis::operator()): Remove. * gimple-predicate-analysis.cc (uninit_analysis::collect_phi_def_edges): Use phi_arg_set, simplify a bit. * tree-ssa-uninit.cc (defined_args): New global. (compute_uninit_opnds_pos): Mask with the recorded set of guarded maybe-uninitialized uses. (uninit_undef_val_t::operator()): Remove. (find_uninit_use): Process all PHI uses, recording the guarded ones and marking the PHI result as uninitialized consistently. (warn_uninitialized_phi): Adjust. (execute_late_warn_uninitialized): Get rid of the PHI worklist and instead walk the function in RPO order. * expmed.cc (store_bit_field_1): Initialize regnum. * spellcheck.h (best_match::m_best_candidate_len): Initialize. --- gcc/expmed.cc | 2 +- gcc/gimple-predicate-analysis.cc | 49 +++++----- gcc/gimple-predicate-analysis.h | 2 - gcc/spellcheck.h | 3 +- gcc/tree-ssa-uninit.cc | 149 +++++++++++++------------------ 5 files changed, 83 insertions(+), 122 deletions(-) diff --git a/gcc/expmed.cc b/gcc/expmed.cc index 8d7418be418..cdc0adb3892 100644 --- a/gcc/expmed.cc +++ b/gcc/expmed.cc @@ -794,7 +794,7 @@ store_bit_field_1 (rtx str_rtx, poly_uint64 bitsize, poly_uint64 bitnum, words or to cope with mode punning between equal-sized modes. In the latter case, use subreg on the rhs side, not lhs. */ rtx sub; - HOST_WIDE_INT regnum; + HOST_WIDE_INT regnum = 0; poly_uint64 regsize = REGMODE_NATURAL_SIZE (GET_MODE (op0)); if (known_eq (bitnum, 0U) && known_eq (bitsize, GET_MODE_BITSIZE (GET_MODE (op0)))) diff --git a/gcc/gimple-predicate-analysis.cc b/gcc/gimple-predicate-analysis.cc index e388bb37685..3f9b80d9128 100644 --- a/gcc/gimple-predicate-analysis.cc +++ b/gcc/gimple-predicate-analysis.cc @@ -543,35 +543,13 @@ uninit_analysis::collect_phi_def_edges (gphi *phi, basic_block cd_root, return; unsigned n = gimple_phi_num_args (phi); + unsigned opnds_arg_phi = m_eval.phi_arg_set (phi); for (unsigned i = 0; i < n; i++) { - edge opnd_edge = gimple_phi_arg_edge (phi, i); - tree opnd = gimple_phi_arg_def (phi, i); - - if (TREE_CODE (opnd) == SSA_NAME) - { - gimple *def = SSA_NAME_DEF_STMT (opnd); - - if (!m_eval (opnd)) - { - if (dump_file && (dump_flags & TDF_DETAILS)) - { - fprintf (dump_file, - "\tFound def edge %i -> %i for cd_root %i " - "and operand %u of: ", - opnd_edge->src->index, opnd_edge->dest->index, - cd_root->index, i); - print_gimple_stmt (dump_file, phi, 0); - } - edges->safe_push (opnd_edge); - } - else if (gimple_code (def) == GIMPLE_PHI - && dominated_by_p (CDI_DOMINATORS, gimple_bb (def), cd_root)) - collect_phi_def_edges (as_a (def), cd_root, edges, - visited); - } - else + if (!MASK_TEST_BIT (opnds_arg_phi, i)) { + /* Add the edge for a not maybe-undefined edge value. */ + edge opnd_edge = gimple_phi_arg_edge (phi, i); if (dump_file && (dump_flags & TDF_DETAILS)) { fprintf (dump_file, @@ -581,9 +559,22 @@ uninit_analysis::collect_phi_def_edges (gphi *phi, basic_block cd_root, cd_root->index, i); print_gimple_stmt (dump_file, phi, 0); } - - if (!m_eval (opnd)) - edges->safe_push (opnd_edge); + edges->safe_push (opnd_edge); + continue; + } + else + { + tree opnd = gimple_phi_arg_def (phi, i); + if (TREE_CODE (opnd) == SSA_NAME) + { + gimple *def = SSA_NAME_DEF_STMT (opnd); + if (gimple_code (def) == GIMPLE_PHI + && dominated_by_p (CDI_DOMINATORS, gimple_bb (def), cd_root)) + /* Process PHI defs of maybe-undefined edge values + recursively. */ + collect_phi_def_edges (as_a (def), cd_root, edges, + visited); + } } } } diff --git a/gcc/gimple-predicate-analysis.h b/gcc/gimple-predicate-analysis.h index 1ca6ab1f7e4..48da8f70efd 100644 --- a/gcc/gimple-predicate-analysis.h +++ b/gcc/gimple-predicate-analysis.h @@ -104,8 +104,6 @@ class uninit_analysis { typedef unsigned phi_arg_set_t; - /* Return true if the argument is an expression of interest. */ - virtual bool operator()(tree) = 0; /* Return a bitset of PHI arguments of interest. By default returns bitset with a bit set for each argument. Should be called in the overriden function first and, if nonzero, the result then 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-uninit.cc b/gcc/tree-ssa-uninit.cc index 450ff0fd062..3e5816f1ebb 100644 --- a/gcc/tree-ssa-uninit.cc +++ b/gcc/tree-ssa-uninit.cc @@ -56,7 +56,8 @@ along with GCC; see the file COPYING3. If not see /* Pointer set of potentially undefined ssa names, i.e., ssa names that are defined by phi with operands that are not defined or potentially undefined. */ -static hash_set *possibly_undefined_names = 0; +static hash_set *possibly_undefined_names; +static hash_map *defined_args; /* Returns the first bit position (starting from LSB) in mask that is non zero. Returns -1 if the mask is empty. */ @@ -1131,6 +1132,9 @@ compute_uninit_opnds_pos (gphi *phi) MASK_SET_BIT (uninit_opnds, i); } } + /* If we have recorded guarded uses of may-uninit values mask those. */ + if (auto *def_mask = defined_args->get (phi)) + uninit_opnds &= ~*def_mask; return uninit_opnds; } @@ -1139,21 +1143,9 @@ compute_uninit_opnds_pos (gphi *phi) struct uninit_undef_val_t: public uninit_analysis::func_t { - virtual bool operator()(tree) override; virtual unsigned phi_arg_set (gphi *) override; }; -/* Return true if the argument is an expression of interest. */ - -bool -uninit_undef_val_t::operator()(tree val) -{ - if (TREE_CODE (val) == SSA_NAME) - return uninit_undefined_value_p (val); - - return false; -} - /* Return a bitset of PHI arguments of interest. */ unsigned @@ -1166,14 +1158,10 @@ uninit_undef_val_t::phi_arg_set (gphi *phi) uninitialized variable defined by PHI and returns a use statement if the use is not properly guarded. It returns NULL if all uses are guarded. UNINIT_OPNDS is a bitvector - holding the position(s) of uninit PHI operands. WORKLIST - is the vector of candidate phis that may be updated by this - function. ADDED_TO_WORKLIST is the pointer set tracking - if the new phi is already in the worklist. */ + holding the position(s) of uninit PHI operands. */ static gimple * -find_uninit_use (gphi *phi, unsigned uninit_opnds, - vec *worklist, hash_set *added_to_worklist) +find_uninit_use (gphi *phi, unsigned uninit_opnds) { /* The Boolean predicate guarding the PHI definition. Initialized lazily from PHI in the first call to is_use_guarded() and cached @@ -1184,6 +1172,7 @@ find_uninit_use (gphi *phi, unsigned uninit_opnds, use_operand_p use_p; imm_use_iterator iter; tree phi_result = gimple_phi_result (phi); + gimple *uninit_use = NULL; FOR_EACH_IMM_USE_FAST (use_p, iter, phi_result) { gimple *use_stmt = USE_STMT (use_p); @@ -1202,11 +1191,32 @@ find_uninit_use (gphi *phi, unsigned uninit_opnds, if (e->flags & EDGE_DFS_BACK) continue; } + else if (uninit_use) + /* Only process the first real uninitialized use, but continue + looking for unguarded uses in PHIs. */ + continue; else use_bb = gimple_bb (use_stmt); if (def_preds.is_use_guarded (use_stmt, use_bb, phi, uninit_opnds)) - continue; + { + /* For a guarded use in a PHI record the PHI argument as + initialized. */ + if (gphi *use_phi = dyn_cast (use_stmt)) + { + unsigned idx = PHI_ARG_INDEX_FROM_USE (use_p); + if (idx < uninit_analysis::func_t::max_phi_args) + { + bool existed_p; + auto &def_mask + = defined_args->get_or_insert (use_phi, &existed_p); + if (!existed_p) + def_mask = 0; + MASK_SET_BIT (def_mask, idx); + } + } + continue; + } if (dump_file && (dump_flags & TDF_DETAILS)) { @@ -1214,59 +1224,35 @@ find_uninit_use (gphi *phi, unsigned uninit_opnds, use_bb->index); print_gimple_stmt (dump_file, use_stmt, 0); } - /* Found one real use, return. */ - if (gimple_code (use_stmt) != GIMPLE_PHI) - return use_stmt; - - /* Found a phi use that is not guarded, - add the phi to the worklist. */ - if (!added_to_worklist->add (as_a (use_stmt))) - { - if (dump_file && (dump_flags & TDF_DETAILS)) - { - fprintf (dump_file, "[WORKLIST]: Update worklist with phi: "); - print_gimple_stmt (dump_file, use_stmt, 0); - } - - worklist->safe_push (as_a (use_stmt)); - possibly_undefined_names->add (phi_result); - } + /* Found a phi use that is not guarded, mark the phi_result as + possibly undefined. */ + if (is_a (use_stmt)) + possibly_undefined_names->add (phi_result); + else + uninit_use = use_stmt; } - return NULL; + return uninit_use; } /* Look for inputs to PHI that are SSA_NAMEs that have empty definitions and gives warning if there exists a runtime path from the entry to a use of the PHI def that does not contain a definition. In other words, the warning is on the real use. The more dead paths that can be pruned - by the compiler, the fewer false positives the warning is. WORKLIST - is a vector of candidate phis to be examined. ADDED_TO_WORKLIST is - a pointer set tracking if the new phi is added to the worklist or not. */ + by the compiler, the fewer false positives the warning is. */ static void -warn_uninitialized_phi (gphi *phi, vec *worklist, - hash_set *added_to_worklist) +warn_uninitialized_phi (gphi *phi, unsigned uninit_opnds) { - /* Don't look at virtual operands. */ - if (virtual_operand_p (gimple_phi_result (phi))) - return; - - unsigned uninit_opnds = compute_uninit_opnds_pos (phi); - if (MASK_EMPTY (uninit_opnds)) - return; - if (dump_file && (dump_flags & TDF_DETAILS)) { fprintf (dump_file, "Examining phi: "); print_gimple_stmt (dump_file, phi, 0); } - gimple *uninit_use_stmt = find_uninit_use (phi, uninit_opnds, - worklist, added_to_worklist); + gimple *uninit_use_stmt = find_uninit_use (phi, uninit_opnds); - /* All uses are properly guarded but a new PHI may have been added - to WORKLIST. */ + /* All uses are properly guarded. */ if (!uninit_use_stmt) return; @@ -1341,10 +1327,6 @@ public: static void execute_late_warn_uninitialized (function *fun) { - basic_block bb; - gphi_iterator gsi; - vec worklist = vNULL; - calculate_dominance_info (CDI_DOMINATORS); calculate_dominance_info (CDI_POST_DOMINATORS); @@ -1352,6 +1334,8 @@ execute_late_warn_uninitialized (function *fun) unreachable blocks. */ set_all_edges_as_executable (fun); mark_dfs_back_edges (fun); + int *rpo = XNEWVEC (int, n_basic_blocks_for_fn (fun)); + int n = pre_and_rev_post_order_compute_fn (fun, NULL, rpo, false); /* Re-do the plain uninitialized variable check, as optimization may have straightened control flow. Do this first so that we don't accidentally @@ -1361,11 +1345,15 @@ execute_late_warn_uninitialized (function *fun) timevar_push (TV_TREE_UNINIT); possibly_undefined_names = new hash_set; - hash_set added_to_worklist; - - /* Initialize worklist */ - FOR_EACH_BB_FN (bb, fun) - for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi)) + defined_args = new hash_map; + + /* Walk the CFG in RPO order so we visit PHIs with defs that are + possibly uninitialized from other PHIs after those. The uninit + predicate analysis will then expand the PHIs predicate with + the predicates of the edges from such PHI defs. */ + for (int i = 0; i < n; ++i) + for (auto gsi = gsi_start_phis (BASIC_BLOCK_FOR_FN (fun, rpo[i])); + !gsi_end_p (gsi); gsi_next (&gsi)) { gphi *phi = gsi.phi (); @@ -1373,35 +1361,18 @@ execute_late_warn_uninitialized (function *fun) if (virtual_operand_p (gimple_phi_result (phi))) continue; - unsigned n = gimple_phi_num_args (phi); - for (unsigned i = 0; i < n; ++i) - { - tree op = gimple_phi_arg_def (phi, i); - if (TREE_CODE (op) == SSA_NAME && uninit_undefined_value_p (op)) - { - worklist.safe_push (phi); - added_to_worklist.add (phi); - if (dump_file && (dump_flags & TDF_DETAILS)) - { - fprintf (dump_file, "[WORKLIST]: add to initial list " - "for operand %u of: ", i); - print_gimple_stmt (dump_file, phi, 0); - } - break; - } - } - } + unsigned uninit_opnds = compute_uninit_opnds_pos (phi); + if (MASK_EMPTY (uninit_opnds)) + continue; - while (worklist.length () != 0) - { - gphi *cur_phi = 0; - cur_phi = worklist.pop (); - warn_uninitialized_phi (cur_phi, &worklist, &added_to_worklist); - } + warn_uninitialized_phi (phi, uninit_opnds); + } - worklist.release (); + free (rpo); delete possibly_undefined_names; possibly_undefined_names = NULL; + delete defined_args; + defined_args = NULL; free_dominance_info (CDI_POST_DOMINATORS); timevar_pop (TV_TREE_UNINIT); } -- 2.35.3