Re: Incremental updating of SSA_NAMEs that are passed to b_c_p

[Jeff Law <law@redhat.com>]

On 10/27/20 12:34 PM, Ilya Leoshkevich wrote:
> Hi,
>
> I'd like to revive the old discussion regarding the interaction of
> jump threading and b_c_p causing the latter to incorrectly return 1 in
> certain cases:
>
> https://gcc.gnu.org/pipermail/gcc-patches/2020-June/547236.html
> https://gcc.gnu.org/pipermail/gcc-patches/2020-July/549288.html
>
> The conclusion was that this happening during threading is just a
> symptom of a deeper problem: SSA_NAMEs that are passed to b_c_p should
> not be registered for incremental updating.
That's perhaps not as precise as we'd like to be.

Simply registering the SSA_NAME isn't the problem.  It's the need to
generate a PHI to resolve the incremental update that causes the
problems.    And as much as anything it's a sanity check that we're not
filtering out jump threading cases that we should (ignore the case for
now where the PHI we generate is a degenerate).

Now it may be painful to actually detect those cases.  I haven't looked
at the into-ssa bits in years.



>
> I performed a little experiment and added an assertion to
> create_new_def_for:
>
> --- a/gcc/tree-into-ssa.c
> +++ b/gcc/tree-into-ssa.c
> @@ -2996,6 +3014,8 @@ create_new_def_for (tree old_name, gimple *stmt,
> def_operand_p def)
>  {
>    tree new_name;
>  
> +  gcc_checking_assert (!used_by_bcp_p (old_name));
> +
>    timevar_push (TV_TREE_SSA_INCREMENTAL);
>  
>    if (!update_ssa_initialized_fn)
>
> This has of course fired when performing basic block duplication during
> threading, which can be fixed by avoiding duplication of basic blocks
> wi
> th b_c_p calls:
>
> --- a/gcc/tree-cfg.c
> +++ b/gcc/tree-cfg.c
> @@ -6224,7 +6224,8 @@ gimple_can_duplicate_bb_p (const_basic_block bb)
>               || gimple_call_internal_p (g, IFN_GOMP_SIMT_EXIT)
>               || gimple_call_internal_p (g, IFN_GOMP_SIMT_VOTE_ANY)
>               || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_BFLY)
> -             || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_IDX)))
> +             || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_IDX)
> +             || gimple_call_builtin_p (g, BUILT_IN_CONSTANT_P)))
>         return false;
>      }
Yea, that's a reasonable place to start with the checking assert to see
how pervasive these problems are.  I probably wouldn't just blindly
disable threading through blocks with b_c_p call, but again, it seems
like a reasonable thing to do to help get a sense of other problem areas.

>
> The second occurrence is a bit more interesting:
>
> gimple *
> vrp_insert::build_assert_expr_for (tree cond, tree v)
> {
>   ...
>   a = build2 (ASSERT_EXPR, TREE_TYPE (v), v, cond);
>   assertion = gimple_build_assign (NULL_TREE, a);
>   ...
>   tree new_def = create_new_def_for (v, assertion, NULL);
>
> The fix is also simple though:
>
> --- a/gcc/tree-vrp.c
> +++ b/gcc/tree-vrp.c
> @@ -3101,6 +3101,9 @@ vrp_insert::process_assert_insertions_for (tree
> name, assert_locus *loc)
>    if (loc->expr == loc->val)
>      return false;
>  
> +  if (used_by_bcp_p (name))
> +    return false;
> +
>    cond = build2 (loc->comp_code, boolean_type_node, loc->expr, loc-
>> val);
>    assert_stmt = build_assert_expr_for (cond, name);
>    if (loc->e)
>
> My original testcase did not trigger anything else.  I'm planning to
> check how this affects the testsuite, but before going further I would
> like to ask: is this the right direction now?  To me it looks a
> little-bit more heavy-handed than the original approach, but maybe it's
> worth it.
It seems  a bit heavy-handed.  But that's not based on any hard data,
just a sense that we may not want to disable threading this aggressively.

jeff