Re: Incremental updating of SSA_NAMEs that are passed to b_c_p

[Richard Biener <richard.guenther@gmail.com>]

On Tue, Oct 27, 2020 at 7:36 PM Ilya Leoshkevich via Gcc
<gcc@gcc.gnu.org> 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.
>
> 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;
>      }
>
> 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.

Disabling threading looks reasonable but I'd rather not disallow BB duplication
or renaming.  For VRP I guess we want to instead change
register_edge_assert_for* to not register assertions for the result of
__builtin_constant_p rather than just not allowing VRP to process it
(there are other consumers still).

Richard.

> Best regards,
> Ilya
>