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

[David Malcolm <dmalcolm at redhat dot com>]

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

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