On Thu, Feb 8, 2018 at 11:23 PM, David Malcolm <dmalc...@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 <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
>
