Re: [PATCH/RFC] tree-if-conv.c: fix two ICEs seen with -fno-tree-forwprop (PR tree-optimization/84178)

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

On Thu, Feb 15, 2018 at 11:07 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> 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

Ick :/

I _think_ that a sensible thing to do would be to make gsi_insert_*
never update stmts when the destination
is not in the IL which means gsi->bb is NULL.  But that's a quite big
change(?).  And we may have code
that relies on stmt operands and immediate uses for out-of IL
sequences (not that I can point to any but
I really wouldn't be surprised).

fold_stmt isn't supposed to mess with SSA operands but obviously it
would be a bad API if it
adds stmts to the IL but not have their operands computed given the
caller only knows whether
the stmt to be folded has been changed but is not passed the set of
emitted stmts.  So trying to
fix the above in fold_stmt itself by more strictly following its
promises is even harder (using
fold_stmt_inplace should behave according to that).

So - can you instead of restoring the original code in place of the assert do

  gimple_seq_discard (stmts);

?

Thanks,
Richard.

> 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&regrtested 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
>> >