From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x72b.google.com (mail-qk1-x72b.google.com [IPv6:2607:f8b0:4864:20::72b]) by sourceware.org (Postfix) with ESMTPS id 1231C3858001 for ; Thu, 2 Sep 2021 16:28:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1231C3858001 Received: by mail-qk1-x72b.google.com with SMTP id bk29so2701387qkb.8 for ; Thu, 02 Sep 2021 09:28:05 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=dc8hdwQRsi4yNl0AGPGgpKGmSPLrctyZ/mNRBx0fW9k=; b=IafcosIlpbMaNcykRzfXsBGo64t6rSGHlMFJg8xyTylSIHnODLSQu2TH+1vY1FahEX gRaChNttX400yPp3eRKCiNXs/abRYqecxqQkVYLHn7fdCYmPDEnJxI3JMLfh6eGM6Y5S rfBDzKRj7VKUUhg77lmPTws3XLpM4HPU2XRKYjUghTpGJfNXWRhAePFeLxqljiNxzPtV WTZNACtBixFu9CwoQt8WzKRgdDXV3fX22Ut+Yt1cccMaeRwtSaHDRv6mcaiyJKY3Rw3e cxrjeS4c7cMJzV8kH9V1vg5ml5cTW1lD/l0o8IznSC3BVNbiCpqM3TCQ99lE66S9QPao eD1A== X-Gm-Message-State: AOAM530RJBJbsDEdWKH/VM20u9C6ht2u1C1L9gfA9HDB/NfU9+jmEe7f ukzhrei/ftewTYxGvlWIAVtouI+beuc= X-Google-Smtp-Source: ABdhPJxy6CxfbHRM3FFRQ1O3HYv4Twwqq24Gner2191mHd4I/5M8WJXlTZ5gxXdpxILwhlmUteNI3w== X-Received: by 2002:a05:620a:799:: with SMTP id 25mr3986947qka.119.1630600084312; Thu, 02 Sep 2021 09:28:04 -0700 (PDT) Received: from [172.31.0.175] (c-98-202-48-222.hsd1.ut.comcast.net. [98.202.48.222]) by smtp.gmail.com with ESMTPSA id r19sm1329483qtt.86.2021.09.02.09.28.02 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 02 Sep 2021 09:28:03 -0700 (PDT) Subject: Re: [PATCH] introduce predicate analysis class To: gcc-patches@gcc.gnu.org References: From: Jeff Law Message-ID: <28c6b053-e3de-afe4-b898-0d610ec7381b@gmail.com> Date: Thu, 2 Sep 2021 10:28:01 -0600 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-Spam-Status: No, score=-3.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, HTML_MESSAGE, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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: Thu, 02 Sep 2021 16:28:15 -0000 On 8/30/2021 2:03 PM, Martin Sebor via Gcc-patches wrote: > The predicate analysis subset of the tree-ssa-uninit pass isn't > necessarily specific to the detection of uninitialized reads. > Suitably parameterized, the same core logic could be used in > other warning passes to improve their S/N ratio, or issue more > nuanced diagnostics (e.g., when an invalid access cannot be > ruled out but also need not in reality be unavoidable, issue > a "may be invalid" type of warning rather than "is invalid"). > > Separating the predicate analysis logic from the uninitialized > pass and exposing a narrow API should also make it easier to > understand and evolve each part independently of the other, > or replace one with a better implementation without modifying > the other.(*) > > As the first step in this direction, the attached patch extracts > the predicate analysis logic out of the pass, turns the interface > into public class members, and hides the internals in either > private members or static functions defined in a new source file. > (**) > > The changes should have no externally observable effect (i.e., > should cause no changes in warnings), except on the contents of > the uninitialized dump.  While making the changes I enhanced > the dumps to help me follow the logic.  Turning some previously > free-standing functions into members involved changing their > signatures and adjusting their callers.  While making these > changes I also renamed some of them as well some variables for > improved clarity.  Finally, I moved declarations of locals > closer to their point of initialization. > > Tested on x86_64-linux.  Besides the usual bootstrap/regtest > I also tentatively verified the generality of the new class > interfaces by making use of it in -Warray-bounds.  Besides there, > I'd like to make use of it in the new gimple-ssa-warn-access pass > and, longer term, any other flow-sensitive warnings that might > benefit from it. > > Martin > > [*] A review of open -Wuninitialized bugs I did while working > on this project made me aware of a number of opportunities to > improve the analyzer to reduce the number of false positives > -Wmaybe-uninitiailzed suffers from. > > [**] The class isn't fully general and, like the uninit pass, > only works with PHI nodes.  I plan to generalize it to compute > the set of predicates between any two basic blocks. > > gcc-predanal.diff > > Factor predidacte analysis out of tree-ssa-uninit.c into its own module. > > gcc/ChangeLog: > > * Makefile.in (OBJS): Add gimple-predicate-analysis.o. > * tree-ssa-uninit.c (max_phi_args): Move to gimple-predicate-analysis. > (MASK_SET_BIT, MASK_TEST_BIT, MASK_EMPTY): Same. > (check_defs): > (can_skip_redundant_opnd): > (compute_uninit_opnds_pos): Adjust to namespace change. > (find_pdom): Move to gimple-predicate-analysis.cc. > (find_dom): Same. > (struct uninit_undef_val_t): New. > (is_non_loop_exit_postdominating): Move to gimple-predicate-analysis.cc. > (find_control_equiv_block): Same. > (MAX_NUM_CHAINS, MAX_CHAIN_LEN, MAX_POSTDOM_CHECK): Same. > (MAX_SWITCH_CASES): Same. > (compute_control_dep_chain): Same. > (find_uninit_use): Use predicate analyzer. > (struct pred_info): Move to gimple-predicate-analysis. > (convert_control_dep_chain_into_preds): Same. > (find_predicates): Same. > (collect_phi_def_edges): Same. > (warn_uninitialized_phi): Use predicate analyzer. > (find_def_preds): Move to gimple-predicate-analysis. > (dump_pred_info): Same. > (dump_pred_chain): Same. > (dump_predicates): Same. > (destroy_predicate_vecs): Remove. > (execute_late_warn_uninitialized): New. > (get_cmp_code): Move to gimple-predicate-analysis. > (is_value_included_in): Same. > (value_sat_pred_p): Same. > (find_matching_predicate_in_rest_chains): Same. > (is_use_properly_guarded): Same. > (prune_uninit_phi_opnds): Same. > (find_var_cmp_const): Same. > (use_pred_not_overlap_with_undef_path_pred): Same. > (pred_equal_p): Same. > (is_neq_relop_p): Same. > (is_neq_zero_form_p): Same. > (pred_expr_equal_p): Same. > (is_pred_expr_subset_of): Same. > (is_pred_chain_subset_of): Same. > (is_included_in): Same. > (is_superset_of): Same. > (pred_neg_p): Same. > (simplify_pred): Same. > (simplify_preds_2): Same. > (simplify_preds_3): Same. > (simplify_preds_4): Same. > (simplify_preds): Same. > (push_pred): Same. > (push_to_worklist): Same. > (get_pred_info_from_cmp): Same. > (is_degenerated_phi): Same. > (normalize_one_pred_1): Same. > (normalize_one_pred): Same. > (normalize_one_pred_chain): Same. > (normalize_preds): Same. > (can_one_predicate_be_invalidated_p): Same. > (can_chain_union_be_invalidated_p): Same. > (uninit_uses_cannot_happen): Same. > (pass_late_warn_uninitialized::execute): Define. > * gimple-predicate-analysis.cc: New file. > * gimple-predicate-analysis.h: New file. Thanks for tackling this.  It's something I think we've needed for a long time. I've only done a cursory review since this is primarily moving the analysis engine to its own file and class-ifying it. As we build additional consumers I won't be surprised if we have to adjust the exposed class API, but that's fine.   The first goal is to get the analysis disentangled from the consumer(s). OK.  Looking forward to seeing the analysis used elsewhere. Jeff