From patchwork Thu Feb 8 22:23:55 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 871150 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-472900-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="NEm9yRVO"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3zcszZ2pLtz9s4q for ; Fri, 9 Feb 2018 09:18:49 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id; q=dns; s=default; b=x/PVKTcBpB26 +YgdA6zXCqlROpjjVSXM7JVTr2uG5CWoffAC16y/tGGWZr35EAyOw5rwIpsazT77 uH2EU4sg+KOz+vFNKsHElrJP+mcXLqEV52LLCQfLcQs3p1FZqqImpIimEBZf7E7W sBCreq3n80WGW6ObDCk16aBWAXq1L/c= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id; s=default; bh=FQ2+h5paEeibyHO9wX foeqBbnz4=; b=NEm9yRVOm0coyIrCm0DGPgqPTVuVM3xb1u5heV5AUEHIVMcOIZ N0yffkM1gGOZKbLdnmhYhHUakMtEdgg2mE3HpBjoz3+sdHzOvs1D7Qq4LGPHHSkN oeQHHwVRY8j6TfKdYIcHBkVvHq2brCdqwPg2KEnaDxogYhy702xge2Vcw= Received: (qmail 53376 invoked by alias); 8 Feb 2018 22:18:42 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 52429 invoked by uid 89); 8 Feb 2018 22:18:41 -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 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; Thu, 08 Feb 2018 22:18:39 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 791EF356D8 for ; Thu, 8 Feb 2018 22:18:38 +0000 (UTC) Received: from c64.redhat.com (ovpn-112-49.phx2.redhat.com [10.3.112.49]) by smtp.corp.redhat.com (Postfix) with ESMTP id A734F5C1AB; Thu, 8 Feb 2018 22:18:37 +0000 (UTC) From: David Malcolm To: gcc-patches@gcc.gnu.org Cc: David Malcolm Subject: [PATCH/RFC] tree-if-conv.c: fix two ICEs seen with -fno-tree-forwprop (PR tree-optimization/84178) Date: Thu, 8 Feb 2018 17:23:55 -0500 Message-Id: <1518128635-15838-1-git-send-email-dmalcolm@redhat.com> X-IsSubscribed: yes 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 * 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®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