Re: [PATCH] introduce predicate analysis class

[Martin Sebor <msebor@gmail.com>]

On 9/2/21 10:28 AM, Jeff Law via Gcc-patches wrote:
> 
> 
> 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.

Thanks, I just pushed it.

As a heads up, I've been having some weird email problems lately and
didn't find your approval until today, in my Trash folder.  Just in
case there's fallout that I'm not responsive to, please either ping
me on IRC or email my @redhat account.

Martin