Re: [PATCH] introduce predicate analysis class

[Martin Sebor <msebor@gmail.com>]

On 9/17/21 10:08 PM, Jeff Law wrote:
> 
> 
> On 9/17/2021 4:05 PM, Martin Sebor wrote:
>> 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.
> Weird.  I wonder if google is trying to tell me something :-)
> 
> Anyway,  it does seem to be triggering an ICE on nios2-linux-gnu and 
> csky-linux-gnu (so far).
> 
> Compile the attached code with -O2 -Wall  with a nios2-linux-gnu cross 
> compiler.  You should get an ICE in the predicate bits.

Thanks.  An assert I added to help me debug the code is firing.
Let me see if it's the assert that's wrong or if I messed up
something else.

Martin