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. Best regards, Ilya
