From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x632.google.com (mail-ej1-x632.google.com [IPv6:2a00:1450:4864:20::632]) by sourceware.org (Postfix) with ESMTPS id D158E3846402 for ; Mon, 5 Jul 2021 11:26:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D158E3846402 Received: by mail-ej1-x632.google.com with SMTP id b2so28621745ejg.8 for ; Mon, 05 Jul 2021 04:26:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=6xPwnABMgE++t7ViT18sgDepQcPpSVHA45PzNNx3PJo=; b=BKaL3jqwxZkJjcxFiN6dn4/k+5MjhFzlQsAAMwgtog5bBw12ikQ6eeHVk5YC0MnqDo QOOB3aLv8l/aU4IKWShNu+U9d4KtxJ1Ktu3Mnyh0UxpBXoYKsbyYFuZNiKaVT31dPZS3 Npma0Fve4LxklwDWQtI7sQ+W/gvX4Zy8wGo7tFNW0GOxBJfQhXunGO5iAbeif+OVq2I8 ibenOfbKxisF7Rl8hyOYfD2obMh16/+oBOVP9FfAawguKk35sp/6Hn7jFv2WWbotVxSR n8Y9T0x730QO/kAwNKQUAd/IpFc+I2xOYU5Jfp/Vysp6gPo5RhzV6lFA38Qm1wtkyiWc GpQg== X-Gm-Message-State: AOAM531PCLfUjpmLy+2w4mDizZHEtEdQCOWmAbXcvnGWzjbp1pfyLcnH /5IbiSMRaCOhqxMxfbdB3HKkH2uqH9gCb7cOz4k= X-Google-Smtp-Source: ABdhPJzRemup7Ti+/3cVr6vc7Dj5eknjdXrheMCEdgwNy7i+xxxO74v21xFPDsSbqPys9B0Ui3Vt1ml2RPrD9seI5U0= X-Received: by 2002:a17:907:3f96:: with SMTP id hr22mr5173875ejc.129.1625484401844; Mon, 05 Jul 2021 04:26:41 -0700 (PDT) MIME-Version: 1.0 References: <1625423864-20417-1-git-send-email-apinski@marvell.com> <1625423864-20417-5-git-send-email-apinski@marvell.com> In-Reply-To: <1625423864-20417-5-git-send-email-apinski@marvell.com> From: Richard Biener Date: Mon, 5 Jul 2021 13:26:30 +0200 Message-ID: Subject: Re: [PATCH 5/5] Port most of the A CMP 0 ? A : -A to match To: Andrew Pinski Cc: GCC Patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 05 Jul 2021 11:26:45 -0000 On Sun, Jul 4, 2021 at 8:42 PM apinski--- via Gcc-patches wrote: > > From: Andrew Pinski > > To improve phiopt and be able to remove abs_replacement, this ports > most of "A CMP 0 ? A : -A" from fold_cond_expr_with_comparison to > match.pd. There is a few extra changes that are needed to remove > the "A CMP 0 ? A : -A" part from fold_cond_expr_with_comparison: > * Need to handle (A - B) case > * Need to handle UN* comparisons. > > I will handle those in a different patch. > > Note phi-opt-15.c test needed to be updated as we get ABSU now > instead of not getting ABS. When ABSU was added phiopt was not > updated even to use ABSU instead of not creating ABS. > > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions. OK > gcc/ChangeLog: > > PR tree-optimization/101039 > * match.pd (A CMP 0 ? A : -A): New patterns. > * tree-ssa-phiopt.c (abs_replacement): Delete function. > (tree_ssa_phiopt_worker): Don't call abs_replacement. > Update comment about abs_replacement. > > gcc/testsuite/ChangeLog: > > PR tree-optimization/101039 > * gcc.dg/tree-ssa/phi-opt-15.c: Update test to expect > ABSU and still not expect ABS_EXPR. > * gcc.dg/tree-ssa/phi-opt-23.c: New test. > * gcc.dg/tree-ssa/phi-opt-24.c: New test. > --- > gcc/match.pd | 60 +++++++++ > gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c | 4 +- > gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c | 44 +++++++ > gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c | 44 +++++++ > gcc/tree-ssa-phiopt.c | 134 +-------------------- > 5 files changed, 152 insertions(+), 134 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c > > diff --git a/gcc/match.pd b/gcc/match.pd > index 4e10d54383c..72860fbd448 100644 > --- a/gcc/match.pd > +++ b/gcc/match.pd > @@ -3976,6 +3976,66 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > (cnd (logical_inverted_value truth_valued_p@0) @1 @2) > (cnd @0 @2 @1))) > > +/* abs/negative simplifications moved from fold_cond_expr_with_comparison, > + Need to handle (A - B) case as fold_cond_expr_with_comparison does. > + Need to handle UN* comparisons. > + > + None of these transformations work for modes with signed > + zeros. If A is +/-0, the first two transformations will > + change the sign of the result (from +0 to -0, or vice > + versa). The last four will fix the sign of the result, > + even though the original expressions could be positive or > + negative, depending on the sign of A. > + > + Note that all these transformations are correct if A is > + NaN, since the two alternatives (A and -A) are also NaNs. */ > + > +(for cnd (cond vec_cond) > + /* A == 0 ? A : -A same as -A */ > + (for cmp (eq uneq) > + (simplify > + (cnd (cmp @0 zerop) @0 (negate@1 @0)) > + (if (!HONOR_SIGNED_ZEROS (type)) > + @1)) > + (simplify > + (cnd (cmp @0 zerop) integer_zerop (negate@1 @0)) > + (if (!HONOR_SIGNED_ZEROS (type)) > + @1)) > + ) > + /* A != 0 ? A : -A same as A */ > + (for cmp (ne ltgt) > + (simplify > + (cnd (cmp @0 zerop) @0 (negate @0)) > + (if (!HONOR_SIGNED_ZEROS (type)) > + @0)) > + (simplify > + (cnd (cmp @0 zerop) @0 integer_zerop) > + (if (!HONOR_SIGNED_ZEROS (type)) > + @0)) > + ) > + /* A >=/> 0 ? A : -A same as abs (A) */ > + (for cmp (ge gt) > + (simplify > + (cnd (cmp @0 zerop) @0 (negate @0)) > + (if (!HONOR_SIGNED_ZEROS (type) > + && !TYPE_UNSIGNED (type)) > + (abs @0)))) > + /* A <=/< 0 ? A : -A same as -abs (A) */ > + (for cmp (le lt) > + (simplify > + (cnd (cmp @0 zerop) @0 (negate @0)) > + (if (!HONOR_SIGNED_ZEROS (type) > + && !TYPE_UNSIGNED (type)) > + (if (ANY_INTEGRAL_TYPE_P (type) > + && !TYPE_OVERFLOW_WRAPS (type)) > + (with { > + tree utype = unsigned_type_for (type); > + } > + (convert (negate (absu:utype @0)))) > + (negate (abs @0))))) > + ) > +) > + > /* -(type)!A -> (type)A - 1. */ > (simplify > (negate (convert?:s (logical_inverted_value:s @0))) > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c > index ac3018ef533..6aec68961cf 100644 > --- a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c > +++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-15.c > @@ -9,4 +9,6 @@ foo (int i) > return i; > } > > -/* { dg-final { scan-tree-dump-not "ABS" "optimized" } } */ > +/* We should not have ABS_EXPR but ABSU_EXPR instead. */ > +/* { dg-final { scan-tree-dump-not "ABS_EXPR" "optimized" } } */ > +/* { dg-final { scan-tree-dump "ABSU" "optimized" } } */ > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c > new file mode 100644 > index 00000000000..ff658cd16a7 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-23.c > @@ -0,0 +1,44 @@ > +/* { dg-options "-O2 -fdump-tree-phiopt" } */ > + > +int f0(int A) > +{ > +// A == 0? A : -A same as -A > + if (A == 0) return A; > + return -A; > +} > + > +int f1(int A) > +{ > +// A != 0? A : -A same as A > + if (A != 0) return A; > + return -A; > +} > +int f2(int A) > +{ > +// A >= 0? A : -A same as abs (A) > + if (A >= 0) return A; > + return -A; > +} > +int f3(int A) > +{ > +// A > 0? A : -A same as abs (A) > + if (A > 0) return A; > + return -A; > +} > +int f4(int A) > +{ > +// A <= 0? A : -A same as -abs (A) > + if (A <= 0) return A; > + return -A; > +} > +int f5(int A) > +{ > +// A < 0? A : -A same as -abs (A) > + if (A < 0) return A; > + return -A; > +} > + > +/* These should be optimized in phiopt1 but is confused by predicts. */ > +/* { dg-final { scan-tree-dump-not "if" "phiopt1" { xfail *-*-* } } } */ > +/* { dg-final { scan-tree-dump-not "if" "phiopt2" } } */ > + > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c > new file mode 100644 > index 00000000000..eb89decb4bf > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/tree-ssa/phi-opt-24.c > @@ -0,0 +1,44 @@ > +/* { dg-options "-O2 -fno-signed-zeros -fdump-tree-phiopt" } */ > + > +float f0(float A) > +{ > +// A == 0? A : -A same as -A > + if (A == 0) return A; > + return -A; > +} > + > +float f1(float A) > +{ > +// A != 0? A : -A same as A > + if (A != 0) return A; > + return -A; > +} > +float f2(float A) > +{ > +// A >= 0? A : -A same as abs (A) > + if (A >= 0) return A; > + return -A; > +} > +float f3(float A) > +{ > +// A > 0? A : -A same as abs (A) > + if (A > 0) return A; > + return -A; > +} > +float f4(float A) > +{ > +// A <= 0? A : -A same as -abs (A) > + if (A <= 0) return A; > + return -A; > +} > +float f5(float A) > +{ > +// A < 0? A : -A same as -abs (A) > + if (A < 0) return A; > + return -A; > +} > + > +/* These should be optimized in phiopt1 but is confused by predicts. */ > +/* { dg-final { scan-tree-dump-not "if" "phiopt1" { xfail *-*-* } } } */ > +/* { dg-final { scan-tree-dump-not "if" "phiopt2" } } */ > + > diff --git a/gcc/tree-ssa-phiopt.c b/gcc/tree-ssa-phiopt.c > index fec8c02c062..a1664c1f914 100644 > --- a/gcc/tree-ssa-phiopt.c > +++ b/gcc/tree-ssa-phiopt.c > @@ -64,8 +64,6 @@ static int value_replacement (basic_block, basic_block, > edge, edge, gphi *, tree, tree); > static bool minmax_replacement (basic_block, basic_block, > edge, edge, gphi *, tree, tree); > -static bool abs_replacement (basic_block, basic_block, > - edge, edge, gphi *, tree, tree); > static bool spaceship_replacement (basic_block, basic_block, > edge, edge, gphi *, tree, tree); > static bool cond_removal_in_popcount_clz_ctz_pattern (basic_block, basic_block, > @@ -352,8 +350,6 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads, bool early_p) > arg0, arg1, > early_p)) > cfgchanged = true; > - else if (abs_replacement (bb, bb1, e1, e2, phi, arg0, arg1)) > - cfgchanged = true; > else if (!early_p > && cond_removal_in_popcount_clz_ctz_pattern (bb, bb1, e1, > e2, phi, arg0, > @@ -2614,134 +2610,6 @@ cond_removal_in_popcount_clz_ctz_pattern (basic_block cond_bb, > return true; > } > > -/* The function absolute_replacement does the main work of doing the absolute > - replacement. Return true if the replacement is done. Otherwise return > - false. > - bb is the basic block where the replacement is going to be done on. arg0 > - is argument 0 from the phi. Likewise for arg1. */ > - > -static bool > -abs_replacement (basic_block cond_bb, basic_block middle_bb, > - edge e0 ATTRIBUTE_UNUSED, edge e1, > - gphi *phi, tree arg0, tree arg1) > -{ > - tree result; > - gassign *new_stmt; > - gimple *cond; > - gimple_stmt_iterator gsi; > - edge true_edge, false_edge; > - gimple *assign; > - edge e; > - tree rhs, lhs; > - bool negate; > - enum tree_code cond_code; > - > - /* If the type says honor signed zeros we cannot do this > - optimization. */ > - if (HONOR_SIGNED_ZEROS (arg1)) > - return false; > - > - /* OTHER_BLOCK must have only one executable statement which must have the > - form arg0 = -arg1 or arg1 = -arg0. */ > - > - assign = last_and_only_stmt (middle_bb); > - /* If we did not find the proper negation assignment, then we cannot > - optimize. */ > - if (assign == NULL) > - return false; > - > - /* If we got here, then we have found the only executable statement > - in OTHER_BLOCK. If it is anything other than arg = -arg1 or > - arg1 = -arg0, then we cannot optimize. */ > - if (gimple_code (assign) != GIMPLE_ASSIGN) > - return false; > - > - lhs = gimple_assign_lhs (assign); > - > - if (gimple_assign_rhs_code (assign) != NEGATE_EXPR) > - return false; > - > - rhs = gimple_assign_rhs1 (assign); > - > - /* The assignment has to be arg0 = -arg1 or arg1 = -arg0. */ > - if (!(lhs == arg0 && rhs == arg1) > - && !(lhs == arg1 && rhs == arg0)) > - return false; > - > - cond = last_stmt (cond_bb); > - result = PHI_RESULT (phi); > - > - /* Only relationals comparing arg[01] against zero are interesting. */ > - cond_code = gimple_cond_code (cond); > - if (cond_code != GT_EXPR && cond_code != GE_EXPR > - && cond_code != LT_EXPR && cond_code != LE_EXPR) > - return false; > - > - /* Make sure the conditional is arg[01] OP y. */ > - if (gimple_cond_lhs (cond) != rhs) > - return false; > - > - if (FLOAT_TYPE_P (TREE_TYPE (gimple_cond_rhs (cond))) > - ? real_zerop (gimple_cond_rhs (cond)) > - : integer_zerop (gimple_cond_rhs (cond))) > - ; > - else > - return false; > - > - /* We need to know which is the true edge and which is the false > - edge so that we know if have abs or negative abs. */ > - extract_true_false_edges_from_block (cond_bb, &true_edge, &false_edge); > - > - /* For GT_EXPR/GE_EXPR, if the true edge goes to OTHER_BLOCK, then we > - will need to negate the result. Similarly for LT_EXPR/LE_EXPR if > - the false edge goes to OTHER_BLOCK. */ > - if (cond_code == GT_EXPR || cond_code == GE_EXPR) > - e = true_edge; > - else > - e = false_edge; > - > - if (e->dest == middle_bb) > - negate = true; > - else > - negate = false; > - > - /* If the code negates only iff positive then make sure to not > - introduce undefined behavior when negating or computing the absolute. > - ??? We could use range info if present to check for arg1 == INT_MIN. */ > - if (negate > - && (ANY_INTEGRAL_TYPE_P (TREE_TYPE (arg1)) > - && ! TYPE_OVERFLOW_WRAPS (TREE_TYPE (arg1)))) > - return false; > - > - result = duplicate_ssa_name (result, NULL); > - > - if (negate) > - lhs = make_ssa_name (TREE_TYPE (result)); > - else > - lhs = result; > - > - /* Build the modify expression with abs expression. */ > - new_stmt = gimple_build_assign (lhs, ABS_EXPR, rhs); > - > - gsi = gsi_last_bb (cond_bb); > - gsi_insert_before (&gsi, new_stmt, GSI_NEW_STMT); > - > - if (negate) > - { > - /* Get the right GSI. We want to insert after the recently > - added ABS_EXPR statement (which we know is the first statement > - in the block. */ > - new_stmt = gimple_build_assign (result, NEGATE_EXPR, lhs); > - > - gsi_insert_after (&gsi, new_stmt, GSI_NEW_STMT); > - } > - > - replace_phi_edge_with_variable (cond_bb, e1, phi, result); > - > - /* Note that we optimized this PHI. */ > - return true; > -} > - > /* Auxiliary functions to determine the set of memory accesses which > can't trap because they are preceded by accesses to the same memory > portion. We do that for MEM_REFs, so we only need to track > @@ -3678,7 +3546,7 @@ gate_hoist_loads (void) > ABS Replacement > --------------- > > - This transformation, implemented in abs_replacement, replaces > + This transformation, implemented in match_simplify_replacement, replaces > > bb0: > if (a >= 0) goto bb2; else goto bb1; > -- > 2.27.0 >