From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 60968 invoked by alias); 7 Mar 2018 18:50:07 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 58323 invoked by uid 89); 7 Mar 2018 18:50:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=h4, sk:avx2-ve, sk:avx2ve, promises X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 07 Mar 2018 18:50:03 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 82ED580F7C; Wed, 7 Mar 2018 18:50:02 +0000 (UTC) Received: from c64.redhat.com (ovpn-112-12.phx2.redhat.com [10.3.112.12]) by smtp.corp.redhat.com (Postfix) with ESMTP id 21F526031E; Wed, 7 Mar 2018 18:50:00 +0000 (UTC) From: David Malcolm To: Richard Biener Cc: GCC Patches , David Malcolm Subject: Re: [PATCH/RFC] tree-if-conv.c: fix two ICEs seen with -fno-tree-forwprop (PR tree-optimization/84178) Date: Wed, 07 Mar 2018 18:50:00 -0000 Message-Id: <1520448976-55841-1-git-send-email-dmalcolm@redhat.com> In-Reply-To: References: X-IsSubscribed: yes X-SW-Source: 2018-03/txt/msg00342.txt.bz2 On Fri, 2018-02-16 at 12:48 +0100, Richard Biener wrote: > On Thu, Feb 15, 2018 at 11:07 PM, David Malcolm > wrote: > > On Fri, 2018-02-09 at 12:02 +0100, Richard Biener wrote: > > > On Thu, Feb 8, 2018 at 11:23 PM, David Malcolm > > 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= > 0x7ffff1a196c0>, stmt=) > > at ../../src/gcc/ssa-iterators.h:307 > > #1 0x00000000012531c5 in add_use_op (fn=0x7ffff1a03000, > > stmt=, op=0x7ffff1a236d8, > > last=0x7fffffffcb10) at ../../src/gcc/tree-ssa-operands.c:307 > > #2 0x0000000001253607 in finalize_ssa_uses (fn=0x7ffff1a03000, > > stmt=) > > at ../../src/gcc/tree-ssa-operands.c:410 > > #3 0x000000000125368b in finalize_ssa_stmt_operands > > (fn=0x7ffff1a03000, stmt=) > > at ../../src/gcc/tree-ssa-operands.c:436 > > #4 0x0000000001254b62 in build_ssa_operands (fn=0x7ffff1a03000, > > stmt=) > > at ../../src/gcc/tree-ssa-operands.c:948 > > #5 0x00000000012550df in update_stmt_operands (fn=0x7ffff1a03000, > > stmt=) > > at ../../src/gcc/tree-ssa-operands.c:1081 > > #6 0x0000000000c10642 in update_stmt_if_modified (s= > 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 ) > > 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 , 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= > 0x7ffff1a26230>, seq_p=0x7fffffffd910) at > > ../../src/gcc/gimplify.c:441 > > #17 0x0000000000c4cc89 in internal_get_tmp_var (val= > 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= > 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 , 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 , > > fallback=1) at ../../src/gcc/gimplify.c:12160 > > #21 0x0000000000c83de5 in force_gimple_operand_1 > > (expr=, stmts=0x7fffffffd910, > > gimple_test_f=0xc0ef75 , > > var=) at ../../src/gcc/gimplify-me.c:78 > > #22 0x00000000010c6387 in add_to_predicate_list > > (loop=0x7ffff1a0a330, bb=, > > nc=) at ../../src/gcc/tree-if- > > conv.c:535 > > #23 0x00000000010c6480 in add_to_dst_predicate_list > > (loop=0x7ffff1a0a330, e= 10)>, > > prev_cond=, cond= > 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= > 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 > > > > > > > > * 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