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 >
