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
