This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH/RFC] tree-if-conv.c: fix two ICEs seen with -fno-tree-forwprop (PR tree-optimization/84178)
On Fri, 2018-02-09 at 12:02 +0100, Richard Biener wrote:
> On Thu, Feb 8, 2018 at 11:23 PM, David Malcolm <dmalcolm@redhat.com>
> wrote:
> > PR tree-optimization/84178 reports a couple of source files that
> > ICE inside
> > ifcvt when compiled with -03 -fno-tree-forwprop (trunk and gcc 7).
> >
> > Both cases involve problems with ifcvt's per-BB gimplified
> > predicates.
> >
> > Testcase 1 fails this assertion within release_bb_predicate during
> > cleanup:
> >
> > 283 if (flag_checking)
> > 284 for (gimple_stmt_iterator i = gsi_start (stmts);
> > 285 !gsi_end_p (i); gsi_next (&i))
> > 286 gcc_assert (! gimple_use_ops (gsi_stmt (i)));
> >
> > The testcase contains a division in the loop, which leads to
> > if_convertible_loop_p returning false (due to gimple_could_trap_p
> > being true
> > for the division). This happens *after* the per-BB gimplified
> > predicates
> > have been created in predicate_bbs (loop).
> > Hence tree_if_conversion bails out to "cleanup", but the gimplified
> > predicates
> > exist and make use of SSA names; for example this conjunction for
> > two BB
> > conditions:
> >
> > _4 = h4.1_112 != 0;
> > _175 = (signed char) _117;
> > _176 = _175 >= 0;
> > _174 = _4 & _176;
> >
> > is using SSA names.
>
> But then this shouldn't cause any stmt operands to be created - who
> is calling
> update_stmt () on a stmt using the SSA names? Maybe something calls
> force_gimple_operand_gsi to add to the sequence?
The immediate use is created deep within folding when the gimplified
predicate is created.
Here's the backtrace of exactly where:
(gdb) bt
#0 link_imm_use_stmt (linknode=0x7ffff1a0b8d0, def=<ssa_name 0x7ffff1a196c0>, stmt=<gimple_assign 0x7ffff1a23690>)
at ../../src/gcc/ssa-iterators.h:307
#1 0x00000000012531c5 in add_use_op (fn=0x7ffff1a03000, stmt=<gimple_assign 0x7ffff1a23690>, op=0x7ffff1a236d8,
last=0x7fffffffcb10) at ../../src/gcc/tree-ssa-operands.c:307
#2 0x0000000001253607 in finalize_ssa_uses (fn=0x7ffff1a03000, stmt=<gimple_assign 0x7ffff1a23690>)
at ../../src/gcc/tree-ssa-operands.c:410
#3 0x000000000125368b in finalize_ssa_stmt_operands (fn=0x7ffff1a03000, stmt=<gimple_assign 0x7ffff1a23690>)
at ../../src/gcc/tree-ssa-operands.c:436
#4 0x0000000001254b62 in build_ssa_operands (fn=0x7ffff1a03000, stmt=<gimple_assign 0x7ffff1a23690>)
at ../../src/gcc/tree-ssa-operands.c:948
#5 0x00000000012550df in update_stmt_operands (fn=0x7ffff1a03000, stmt=<gimple_assign 0x7ffff1a23690>)
at ../../src/gcc/tree-ssa-operands.c:1081
#6 0x0000000000c10642 in update_stmt_if_modified (s=<gimple_assign 0x7ffff1a23690>) at ../../src/gcc/gimple-ssa.h:185
#7 0x0000000000c10e82 in update_modified_stmts (seq=0x7ffff1a23690) at ../../src/gcc/gimple-iterator.c:58
#8 0x0000000000c111f1 in gsi_insert_seq_before (i=0x7fffffffcfb0, seq=0x7ffff1a23690, mode=GSI_SAME_STMT)
at ../../src/gcc/gimple-iterator.c:217
#9 0x0000000000c241d0 in replace_stmt_with_simplification (gsi=0x7fffffffcfb0, rcode=..., ops=0x7fffffffcdb0,
seq=0x7fffffffcdd8, inplace=false) at ../../src/gcc/gimple-fold.c:4473
#10 0x0000000000c25a63 in fold_stmt_1 (gsi=0x7fffffffcfb0, inplace=false, valueize=0xc2663b <no_follow_ssa_edges(tree_node*)>)
at ../../src/gcc/gimple-fold.c:4775
#11 0x0000000000c266b7 in fold_stmt (gsi=0x7fffffffcfb0) at ../../src/gcc/gimple-fold.c:4996
#12 0x0000000000c552b1 in maybe_fold_stmt (gsi=0x7fffffffcfb0) at ../../src/gcc/gimplify.c:3193
#13 0x0000000000c5f1e9 in gimplify_modify_expr (expr_p=0x7fffffffd328, pre_p=0x7fffffffd910, post_p=0x7fffffffd1e0,
want_value=false) at ../../src/gcc/gimplify.c:5803
#14 0x0000000000c7b461 in gimplify_expr (expr_p=0x7fffffffd328, pre_p=0x7fffffffd910, post_p=0x7fffffffd1e0,
gimple_test_f=0xc5d723 <is_gimple_stmt(tree)>, fallback=0) at ../../src/gcc/gimplify.c:11434
#15 0x0000000000c62661 in gimplify_stmt (stmt_p=0x7fffffffd328, seq_p=0x7fffffffd910) at ../../src/gcc/gimplify.c:6658
#16 0x0000000000c4c449 in gimplify_and_add (t=<init_expr 0x7ffff1a26230>, seq_p=0x7fffffffd910) at ../../src/gcc/gimplify.c:441
#17 0x0000000000c4cc89 in internal_get_tmp_var (val=<le_expr 0x7ffff1a26140>, pre_p=0x7fffffffd910, post_p=0x0, is_formal=true,
allow_ssa=true) at ../../src/gcc/gimplify.c:597
#18 0x0000000000c4ccd2 in get_formal_tmp_var (val=<le_expr 0x7ffff1a26140>, pre_p=0x7fffffffd910) at ../../src/gcc/gimplify.c:618
#19 0x0000000000c7ee6a in gimplify_expr (expr_p=0x7ffff1a261b0, pre_p=0x7fffffffd910, post_p=0x7fffffffd790,
gimple_test_f=0xc0f6d0 <is_gimple_val(tree_node*)>, fallback=1) at ../../src/gcc/gimplify.c:12383
#20 0x0000000000c7e2e9 in gimplify_expr (expr_p=0x7fffffffd8b8, pre_p=0x7fffffffd910, post_p=0x7fffffffd790,
gimple_test_f=0xc0ef75 <is_gimple_condexpr(tree_node*)>, fallback=1) at ../../src/gcc/gimplify.c:12160
#21 0x0000000000c83de5 in force_gimple_operand_1 (expr=<bit_and_expr 0x7ffff1a26190>, stmts=0x7fffffffd910,
gimple_test_f=0xc0ef75 <is_gimple_condexpr(tree_node*)>, var=<tree 0x0>) at ../../src/gcc/gimplify-me.c:78
#22 0x00000000010c6387 in add_to_predicate_list (loop=0x7ffff1a0a330, bb=<basic_block 0x7ffff1a250d0 (10)>,
nc=<bit_and_expr 0x7ffff1a26190>) at ../../src/gcc/tree-if-conv.c:535
#23 0x00000000010c6480 in add_to_dst_predicate_list (loop=0x7ffff1a0a330, e=<edge 0x7ffff1a10a50 (9 -> 10)>,
prev_cond=<ne_expr 0x7ffff1a26168>, cond=<bit_and_expr 0x7ffff1a26190>) at ../../src/gcc/tree-if-conv.c:557
#24 0x00000000010c7f51 in predicate_bbs (loop=0x7ffff1a0a330) at ../../src/gcc/tree-if-conv.c:1277
#25 0x00000000010c84af in if_convertible_loop_p_1 (loop=0x7ffff1a0a330, refs=0x7fffffffdb08) at ../../src/gcc/tree-if-conv.c:1409
#26 0x00000000010c8aab in if_convertible_loop_p (loop=0x7ffff1a0a330) at ../../src/gcc/tree-if-conv.c:1526
#27 0x00000000010cc7af in tree_if_conversion (loop=0x7ffff1a0a330) at ../../src/gcc/tree-if-conv.c:2833
#28 0x00000000010ccad6 in (anonymous namespace)::pass_if_conversion::execute (this=0x2ae0ba0, fun=0x7ffff1a03000)
at ../../src/gcc/tree-if-conv.c:2962
#29 0x0000000000ecb788 in execute_one_pass (pass=<opt_pass* 0x2ae0ba0 "ifcvt"(165)>) at ../../src/gcc/passes.c:2497
Thoughts?
I'm testing your proposed fix for the other issue now.
Thanks
Dave
> > This assertion was added in r236498 (aka
> > c3deca2519d97c55876869c57cf11ae1e5c6cf8b):
> >
> > 2016-05-20 Richard Biener <rguenther@suse.de>
> >
> > * tree-if-conv.c (add_bb_predicate_gimplified_stmts): Use
> > gimple_seq_add_seq_without_update.
> > (release_bb_predicate): Assert we have no operands to free.
> > (if_convertible_loop_p_1): Calculate post dominators later.
> > Do not free BB predicates here.
> > (combine_blocks): Do not recompute BB predicates.
> > (version_loop_for_if_conversion): Save BB predicates around
> > loop versioning.
> >
> > * gcc.dg/tree-ssa/ifc-cd.c: Adjust.
> >
> > The following patch fixes this by removing the assertion, and
> > reinstating the
> > cleanup of the operands.
>
> But that was supposed to be not necessary... I'll note that simply
> restoring
> the old behavior is not ideal either - luckily we now have
> gimple_seq_discard ()
> which should do a better job here (and actually does what the
> function comment
> says!).
>
> >
> > Testcase 2 segfaults inside update_ssa when called from
> > version_loop_for_if_conversion when a loop is versioned.
> >
> > During loop_version, some blocks are duplicated, and this can
> > duplicate
> > SSA names, leading to the duplicated SSA names being added to
> > tree-into-ssa.c's old_ssa_names.
> >
> > For example, _117 is an SSA name defined in one of these to-be-
> > duplicated
> > blocks, and hence is added to old_ssa_names when duplicated.
> >
> > One of the BBs has this gimplified predicate (again, these stmts
> > are *not*
> > yet in a BB):
> > _173 = h4.1_112 != 0;
> > _171 = (signed char) _117;
> > _172 = _171 >= 0;
> > _170 = ~_172;
> > _169 = _170 & _173;
> >
> > Note the reference to SSA name _117 in the above.
> >
> > When update_ssa runs later on in version_loop_for_if_conversion,
> > SSA name _117 is in the old_ssa_names bitmap, and thus has
> > prepare_use_sites_for called on it:
> >
> > 2771 /* If an old name is in NAMES_TO_RELEASE, we cannot
> > remove it from
> > 2772 OLD_SSA_NAMES, but we have to ignore its definition
> > site. */
> > 2773 EXECUTE_IF_SET_IN_BITMAP (old_ssa_names, 0, i, sbi)
> > 2774 {
> > 2775 if (names_to_release == NULL || !bitmap_bit_p
> > (names_to_release, i))
> > 2776 prepare_def_site_for (ssa_name (i), insert_phi_p);
> > 2777 prepare_use_sites_for (ssa_name (i), insert_phi_p);
> > 2778 }
> >
> > prepare_use_sites_for iterates over the immediate users, which
> > includes
> > the:
> > _171 = (signed char) _117;
> > in the gimplified predicate.
> >
> > This tried to call "mark_block_for_update" on the BB of this
> > def_stmt,
> > leading to a read-through-NULL segfault, since this statement isn't
> > in a
> > BB yet.
> >
> > With the caveat that this is at the limit of my understanding of
> > the
> > innards of gimple, I'm wondering how this ever works: we have
> > gimplified
> > predicates that aren't in a BB yet, which typically refer to
> > SSA names in the CFG proper, and we're attempting non-trivial
> > manipulations
> > of that CFG that can e.g. duplicate those SSA names.
> >
> > The following patch fixes the 2nd ICE by inserting the gimplified
> > predicates
> > earlier: immediately before the possible
> > version_loop_for_if_conversion,
> > rather than within combine_blocks. That way they're in their BBs
> > before
> > we attempt any further manipulation of the CFG.
>
> Hum, but that will alter both copies of the loops, no? I think the
> fix is
> to instead delay the update_ssa call to _after_ combine_blocks ()
> (and remember if it is necessary just in case we didn't version).
>
> Richard.
>
> > This fixes the ICE, though it introduces a regression of
> > gcc.target/i386/avx2-vec-mask-bit-not.c
> > which no longer vectorizes for some reason (I haven't investigated
> > why yet).
> >
> > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu.
> >
> > Thoughts? Does this analysis sound sane?
> >
> > Dave
> >
> > gcc/ChangeLog:
> > PR tree-optimization/84178
> > * tree-if-conv.c (release_bb_predicate): Reinstate the
> > free_stmt_operands loop removed in r236498, removing
> > the assertion that the stmts have NULL use_ops.
> > (combine_blocks): Move the call to
> > insert_gimplified_predicates...
> > (tree_if_conversion): ...to here, immediately before
> > attempting
> > to version the loop.
> >
> > gcc/testsuite/ChangeLog:
> > PR tree-optimization/84178
> > * gcc.c-torture/compile/pr84178-1.c: New test.
> > * gcc.c-torture/compile/pr84178-2.c: New test.
> > ---
> > gcc/testsuite/gcc.c-torture/compile/pr84178-1.c | 18
> > ++++++++++++++++++
> > gcc/testsuite/gcc.c-torture/compile/pr84178-2.c | 18
> > ++++++++++++++++++
> > gcc/tree-if-conv.c | 15 +++++++++--
> > ----
> > 3 files changed, 45 insertions(+), 6 deletions(-)
> > create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr84178-1.c
> > create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr84178-2.c
> >
> > diff --git a/gcc/testsuite/gcc.c-torture/compile/pr84178-1.c
> > b/gcc/testsuite/gcc.c-torture/compile/pr84178-1.c
> > new file mode 100644
> > index 0000000..49f2c89
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.c-torture/compile/pr84178-1.c
> > @@ -0,0 +1,18 @@
> > +/* { dg-options "-fno-tree-forwprop" } */
> > +
> > +int zy, h4;
> > +
> > +void
> > +r8 (long int mu, int *jr, int *fi, short int dv)
> > +{
> > + do
> > + {
> > + int tx;
> > +
> > + tx = !!h4 ? (zy / h4) : 1;
> > + mu = tx;
> > + *jr = (((unsigned char) mu > (254 >> dv)) ? 0 : (unsigned
> > char) tx) + *fi;
> > + } while (*jr == 0);
> > +
> > + r8 (mu, jr, fi, 1);
> > +}
> > diff --git a/gcc/testsuite/gcc.c-torture/compile/pr84178-2.c
> > b/gcc/testsuite/gcc.c-torture/compile/pr84178-2.c
> > new file mode 100644
> > index 0000000..b2548e9
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.c-torture/compile/pr84178-2.c
> > @@ -0,0 +1,18 @@
> > +/* { dg-options "-fno-tree-forwprop" } */
> > +
> > +int zy, h4;
> > +
> > +void
> > +r8 (long int mu, int *jr, int *fi, short int dv)
> > +{
> > + do
> > + {
> > + int tx;
> > +
> > + tx = !!h4 ? (zy + h4) : 1;
> > + mu = tx;
> > + *jr = (((unsigned char) mu > (254 >> dv)) ? 0 : (unsigned
> > char) tx) + *fi;
> > + } while (*jr == 0);
> > +
> > + r8 (mu, jr, fi, 1);
> > +}
> > diff --git a/gcc/tree-if-conv.c b/gcc/tree-if-conv.c
> > index cac3fd7..3edfc70 100644
> > --- a/gcc/tree-if-conv.c
> > +++ b/gcc/tree-if-conv.c
> > @@ -280,11 +280,9 @@ release_bb_predicate (basic_block bb)
> > gimple_seq stmts = bb_predicate_gimplified_stmts (bb);
> > if (stmts)
> > {
> > - if (flag_checking)
> > - for (gimple_stmt_iterator i = gsi_start (stmts);
> > - !gsi_end_p (i); gsi_next (&i))
> > - gcc_assert (! gimple_use_ops (gsi_stmt (i)));
> > -
> > + gimple_stmt_iterator i;
> > + for (i = gsi_start (stmts); !gsi_end_p (i); gsi_next (&i))
> > + free_stmt_operands (cfun, gsi_stmt (i));
> > set_bb_predicate_gimplified_stmts (bb, NULL);
> > }
> > }
> > @@ -2369,7 +2367,6 @@ combine_blocks (struct loop *loop)
> > edge_iterator ei;
> >
> > remove_conditions_and_labels (loop);
> > - insert_gimplified_predicates (loop);
> > predicate_all_scalar_phis (loop);
> >
> > if (any_pred_load_store)
> > @@ -2839,6 +2836,12 @@ tree_if_conversion (struct loop *loop)
> > || loop->dont_vectorize))
> > goto cleanup;
> >
> > + /* We've generated gimplified predicates, but they aren't in
> > their BBs
> > + yet. Put them there now, in case
> > version_loop_for_if_conversion
> > + needs to duplicate the SSA names for the gimplified
> > predicates
> > + (at which point they need to be in BBs; PR 84178). */
> > + insert_gimplified_predicates (loop);
> > +
> > /* Since we have no cost model, always version loops unless the
> > user
> > specified -ftree-loop-if-convert or unless versioning is
> > required.
> > Either version this loop, or if the pattern is right for
> > outer-loop
> > --
> > 1.8.5.3
> >