On Fri, 2018-02-16 at 12:48 +0100, Richard Biener wrote:
> On Thu, Feb 15, 2018 at 11:07 PM, David Malcolm <dmalc...@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.c
> > > om>
> > > 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);
>
> ?
I'm attaching a pair of patches; patch 1 does the above, fixing the
first issue.
>
> Thanks,
> Richard.
>
> > Thoughts?
> >
> > I'm testing your proposed fix for the other issue now.
I wasn't able to get your proposed fix working, so patch 2 merely adds
a test case that triggers the bug.
Dave
> >
> > Thanks
> > Dave
> >
> > > > This assertion was added in r236498 (aka
> > > > c3deca2519d97c55876869c57cf11ae1e5c6cf8b):
> > > >
> > > > 2016-05-20 Richard Biener <rguent...@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
David Malcolm (2):
tree-if-conv.c: fix ICEs seen with -fno-tree-forwprop (PR
tree-optimization/84178)
ifcvt: unfixed testcase for 2nd issue within PR
tree-optimization/84178
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 | 8 +++++---
3 files changed, 41 insertions(+), 3 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
--
1.8.5.3
