diff mbox series

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

Message ID 1518128635-15838-1-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series [PATCH/RFC] tree-if-conv.c: fix two ICEs seen with -fno-tree-forwprop (PR tree-optimization/84178) | expand

Commit Message

David Malcolm Feb. 8, 2018, 10:23 p.m. UTC
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.

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.

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.

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

Comments

Richard Biener Feb. 9, 2018, 11:02 a.m. UTC | #1
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?

> 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
>
David Malcolm Feb. 15, 2018, 10:07 p.m. UTC | #2
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&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
> >
Richard Biener Feb. 16, 2018, 11:48 a.m. UTC | #3
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
>> >
David Malcolm March 7, 2018, 6:56 p.m. UTC | #4
On Fri, 2018-02-16 at 12:48 +0100, Richard Biener wrote:
> 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.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  <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


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
diff mbox series

Patch

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