This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH 1/2] tree-if-conv.c: fix ICE seen with -fno-tree-forwprop (PR tree-optimization/84178)
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: David Malcolm <dmalcolm at redhat dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 8 Mar 2018 09:58:03 +0100
- Subject: Re: [PATCH 1/2] tree-if-conv.c: fix ICE seen with -fno-tree-forwprop (PR tree-optimization/84178)
- Authentication-results: sourceware.org; auth=none
- References: <CAFiYyc16donQnyewupAyfT33qSwz_bakegekKHTL0F8Gd-EP_Q@mail.gmail.com> <1520448976-55841-1-git-send-email-dmalcolm@redhat.com> <1520448976-55841-2-git-send-email-dmalcolm@redhat.com>
On Wed, Mar 7, 2018 at 7:56 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.
>
> 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 adding a call to gimple_seq_discard
> to release_bb_predicate. It also updates the assertion, so that
> instead of asserting the stmts have no imm uses, instead assert that
> they weren't added to a bb before discarding them (otherwise discarding
> them would be a bug). We know this is the case because
> insert_gimplified_predicates has:
>
> /* Once the sequence is code generated, set it to NULL. */
> set_bb_predicate_gimplified_stmts (bb, NULL);
>
> but asserting it seems appropriate as a double-check.
>
> The patch doesn't address the 2nd issue within PR tree-optimization/84178.
>
> Successfully bootstrapped®rtested on x86_64-pc-linux-gnu.
>
> OK for trunk?
OK.
Thanks,
Richard.
> gcc/ChangeLog:
> PR tree-optimization/84178
> * tree-if-conv.c (release_bb_predicate): Remove the
> the assertion that the stmts have NULL use_ops.
> Discard the statements, asserting that they haven't
> yet been added to a BB.
>
> gcc/testsuite/ChangeLog:
> PR tree-optimization/84178
> * gcc.c-torture/compile/pr84178-1.c: New test.
> ---
> gcc/testsuite/gcc.c-torture/compile/pr84178-1.c | 18 ++++++++++++++++++
> gcc/tree-if-conv.c | 8 +++++---
> 2 files changed, 23 insertions(+), 3 deletions(-)
> create mode 100644 gcc/testsuite/gcc.c-torture/compile/pr84178-1.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/tree-if-conv.c b/gcc/tree-if-conv.c
> index cac3fd7..5467f3f 100644
> --- a/gcc/tree-if-conv.c
> +++ b/gcc/tree-if-conv.c
> @@ -271,8 +271,7 @@ init_bb_predicate (basic_block bb)
> set_bb_predicate (bb, boolean_true_node);
> }
>
> -/* Release the SSA_NAMEs associated with the predicate of basic block BB,
> - but don't actually free it. */
> +/* Release the SSA_NAMEs associated with the predicate of basic block BB. */
>
> static inline void
> release_bb_predicate (basic_block bb)
> @@ -280,11 +279,14 @@ release_bb_predicate (basic_block bb)
> gimple_seq stmts = bb_predicate_gimplified_stmts (bb);
> if (stmts)
> {
> + /* Ensure that these stmts haven't yet been added to a bb. */
> 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)));
> + gcc_assert (! gimple_bb (gsi_stmt (i)));
>
> + /* Discard them. */
> + gimple_seq_discard (stmts);
> set_bb_predicate_gimplified_stmts (bb, NULL);
> }
> }
> --
> 1.8.5.3
>