From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x644.google.com (mail-ej1-x644.google.com [IPv6:2a00:1450:4864:20::644]) by sourceware.org (Postfix) with ESMTPS id 453963858012 for ; Wed, 28 Oct 2020 13:02:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 453963858012 Received: by mail-ej1-x644.google.com with SMTP id gs25so7092006ejb.1 for ; Wed, 28 Oct 2020 06:02:05 -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=e2NtfGbvUUbRD96A8z6/LUTFi+8enGDv1DGymMfMfXc=; b=ULv9NvMVqhNGr4mgBRhzNb8BMOYFN76goC3fbo+5grZd6RZVaegNPQa3dmB18RAJy3 n/PYUOvXs7erld8K1shL4aorOTdcoLQVEJZtnKXKOvp6763qbvYUGSHZmYLQYPYqfDMP NOrrPP/beRXIuF8OZOxAm7/hZi0NdYPXcwFwR+kbdZEOkBD0eRiPjciCOazxTsZeEow5 jVb4nj8+hD139cdcTBfD59YiqwiIK/1rg4aIdkzIxqUUE4U2kecsADakVsEU/YR7kp9E Sp4QNTboHVzxxWuvutseHpp7/KjT4SjXcdHIBHxt1UkP+jwCyLMvC5IbjP1GK7lmjIQB cIyQ== X-Gm-Message-State: AOAM531+78m5beTviKHGT9OEdhEGj5EjQi/tfk7BnST3rzxEo6VW0kxO JrVcC9h7d3atA/X9+F4ZMYc8vy+46CXOjBNsU7l1o0mh X-Google-Smtp-Source: ABdhPJxyg5t6rsQkba7XGeZ2kZdhH7fgzJlro/V2/A+7hT+CV1fUjlNwPKk48OWp4yhXpOX2QJOuYj/Vx8VTWR8fSdM= X-Received: by 2002:a17:906:3a97:: with SMTP id y23mr7394164ejd.250.1603890123959; Wed, 28 Oct 2020 06:02:03 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Richard Biener Date: Wed, 28 Oct 2020 14:01:52 +0100 Message-ID: Subject: Re: move sincos after pre To: Alexandre Oliva Cc: GCC Patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-7.8 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.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Wed, 28 Oct 2020 13:02:07 -0000 On Wed, Oct 28, 2020 at 4:18 AM Alexandre Oliva wrote: > > On Oct 27, 2020, Richard Biener wrote: > > > For trapping math SRC may be a constant? Better be safe > > and guard against TREE_CODE (src) != SSA_NAME. > > *nod* > > > You also want to guard against SSA_NAME_OCCURS_IN_ABNORMAL_PHI (src) > > since you cannot generally propagate or move uses of those. > > What if I don't propagate or move them? That's fine then. > In my first cut at this change, I figured it (looked like it) took half > a brain to implement it, and shut down the other half, only making minor > tweaks to a copy of execute_cse_sincos_1. > > Your response was a wake-up call to many issues I hadn't considered but > should have, and that it looks like sincos doesn't either, so I ended up > rewriting the cse_conv function to use and propagate a preexisting > dominating def, instead of inserting one at a point where there wasn't > one. That's because any such conv might trap, unless an earlier one > didn't. > > > Any reason you are not replacing all uses via replace_uses_by > > and removing the old conversion stmts? OK, removing might > > wreck the upthread iterator so replacing with GIMPLE_NOP > > is the usual trick then. > > Removing them would be fine, I was just thoughtlessly mirroring the > cse_sincos_1 behavior that I'd based it on. > > > Now, I put in code to leave conversion results alone if > SSA_NAME_OCCURS_IN_ABNORMAL_PHI (lhs), but I wonder if it would work to > replace uses thereof, or to propagate uses thereof, as long as I turned > such conversion results that would be deleted into copies. I.e., given: > > _2 = (double) _1; > _4 = sin (_2); > ... > _3 = (double) _1; // occurs in abnormal phi below > _5 = cos (_3); > ... > _6 = PHI <..., _3 (abnormal), ...>; > > Wouldn't this be ok to turn into: > > _2 = (double) _1; > _4 = sin (_2); > ... > _3 = _2; > _5 = cos (_2); > ... > _6 = PHI <..., _3 (abnormal), ...>; > > ? Yes. > > Anyway, here's a patch that does *not* assume it would work, and skips > defs used in abnormal phis instead. > > > sincos, however, may mess with them, and even introduce/move a trapping > call to a place where there wasn't any, even if none of the preexisting > calls were going to be executed. The only thing I fixed there was a > plain return within a FOR_EACH_IMM_USE_STMT that I introduced recently. > Though it's unlikely to ever hit, I understand it's wrong per the > current API. > > BTW, any reason why we are not (yet?) using something like: > > #define FOR_EACH_IMM_USE_STMT(STMT, ITER, SSAVAR) \ > for (auto_end_imm_use_stmt_traverse auto_end \ > ((((STMT) = first_imm_use_stmt (&(ITER), (SSAVAR))), \ > &(ITER))); \ > !end_imm_use_stmt_p (&(ITER)); \ > (void) ((STMT) = next_imm_use_stmt (&(ITER)))) Just laziness. Or rather last time I remembered this I tried to do it more fancy via range-for but didn't get very far due to the nested iteration ... > > Anyway, here's what I'm testing now. Bootstrap succeeded, regression > testing underway. Ok to install if it succeeds? OK. Thanks, Richard. > > CSE conversions within sincos > > From: Alexandre Oliva > > On platforms in which Aux_[Real_Type] involves non-NOP conversions > (e.g., between single- and double-precision, or between short float > and float), the conversions before the calls are CSEd too late for > sincos to combine calls. > > This patch enables the sincos pass to CSE type casts used as arguments > to eligible calls before looking for other calls using the same > operand. > > > for gcc/ChangeLog > > * tree-ssa-math-opts.c (sincos_stats): Add conv_removed. > (execute_cse_conv_1): New. > (execute_cse_sincos_1): Call it. Fix return within > FOR_EACH_IMM_USE_STMT. > (pass_cse_sincos::execute): Report conv_inserted. > > for gcc/testsuite/ChangeLog > > * gnat.dg/sin_cos.ads: New. > * gnat.dg/sin_cos.adb: New. > * gcc.dg/sin_cos.c: New. > --- > gcc/testsuite/gcc.dg/sin_cos.c | 41 ++++++++++++++ > gcc/testsuite/gnat.dg/sin_cos.adb | 14 +++++ > gcc/testsuite/gnat.dg/sin_cos.ads | 4 + > gcc/tree-ssa-math-opts.c | 107 +++++++++++++++++++++++++++++++++++++ > 4 files changed, 165 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/gcc.dg/sin_cos.c > create mode 100644 gcc/testsuite/gnat.dg/sin_cos.adb > create mode 100644 gcc/testsuite/gnat.dg/sin_cos.ads > > diff --git a/gcc/testsuite/gcc.dg/sin_cos.c b/gcc/testsuite/gcc.dg/sin_cos.c > new file mode 100644 > index 00000000..aa71dca > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/sin_cos.c > @@ -0,0 +1,41 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2" } */ > + > +/* This maps to essentially the same gimple that is generated for > + gnat.dg/sin_cos.adb, on platforms that use the wraplf variant of > + Ada.Numerics.Aux_Float. The value of EPSILON is not relevant to > + the test, but the test must be there to keep the conversions in > + different BBs long enough to trigger the problem that prevented the > + sincos optimization, because the arguments passed to sin and cos > + didn't get unified into a single SSA_NAME in time for sincos. */ > + > +#include > + > +#define EPSILON 3.4526697709225118160247802734375e-4 > + > +static float my_sinf(float x) { > + return (float) sin ((double) x); > +} > + > +static float wrap_sinf(float x) { > + if (fabs (x) < EPSILON) > + return 0; > + return my_sinf (x); > +} > + > +static float my_cosf(float x) { > + return (float) cos ((double) x); > +} > + > +static float wrap_cosf(float x) { > + if (fabs (x) < EPSILON) > + return 1; > + return my_cosf (x); > +} > + > +float my_sin_cos(float x, float *s, float *c) { > + *s = wrap_sinf (x); > + *c = wrap_cosf (x); > +} > + > +/* { dg-final { scan-assembler "sincos\|cexp" { target *-linux-gnu* *-w64-mingw* *-*-vxworks* } } } */ > diff --git a/gcc/testsuite/gnat.dg/sin_cos.adb b/gcc/testsuite/gnat.dg/sin_cos.adb > new file mode 100644 > index 00000000..6e18df9 > --- /dev/null > +++ b/gcc/testsuite/gnat.dg/sin_cos.adb > @@ -0,0 +1,14 @@ > +-- { dg-do compile } > +-- { dg-options "-O2 -gnatn" } > + > +with Ada.Numerics.Elementary_Functions; > +use Ada.Numerics.Elementary_Functions; > +package body Sin_Cos is > + procedure Sin_Cos (Angle : T; SinA, CosA : out T) is > + begin > + SinA := Sin (Angle); > + CosA := Cos (Angle); > + end; > +end Sin_Cos; > + > +-- { dg-final { scan-assembler "sincos\|cexp" { target *-linux-gnu* *-w64-mingw* *-*-vxworks* } } } > diff --git a/gcc/testsuite/gnat.dg/sin_cos.ads b/gcc/testsuite/gnat.dg/sin_cos.ads > new file mode 100644 > index 00000000..a0eff3d > --- /dev/null > +++ b/gcc/testsuite/gnat.dg/sin_cos.ads > @@ -0,0 +1,4 @@ > +package Sin_Cos is > + subtype T is Float; > + procedure Sin_Cos (Angle : T; SinA, CosA : out T); > +end Sin_Cos; > diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c > index 90dfb98..65c7b34 100644 > --- a/gcc/tree-ssa-math-opts.c > +++ b/gcc/tree-ssa-math-opts.c > @@ -187,6 +187,10 @@ static struct > { > /* Number of cexpi calls inserted. */ > int inserted; > + > + /* Number of conversions removed. */ > + int conv_removed; > + > } sincos_stats; > > static struct > @@ -1099,6 +1103,103 @@ make_pass_cse_reciprocals (gcc::context *ctxt) > return new pass_cse_reciprocals (ctxt); > } > > +/* If NAME is the result of a type conversion, look for other > + equivalent dominating or dominated conversions, and replace all > + uses with the earliest dominating name, removing the redundant > + conversions. Return the prevailing name. */ > + > +static tree > +execute_cse_conv_1 (tree name) > +{ > + if (SSA_NAME_IS_DEFAULT_DEF (name) > + || SSA_NAME_OCCURS_IN_ABNORMAL_PHI (name)) > + return name; > + > + gimple *def_stmt = SSA_NAME_DEF_STMT (name); > + > + if (!gimple_assign_cast_p (def_stmt)) > + return name; > + > + tree src = gimple_assign_rhs1 (def_stmt); > + > + if (TREE_CODE (src) != SSA_NAME) > + return name; > + > + imm_use_iterator use_iter; > + gimple *use_stmt; > + > + /* Find the earliest dominating def. */ > + FOR_EACH_IMM_USE_STMT (use_stmt, use_iter, src) > + { > + if (use_stmt == def_stmt > + || !gimple_assign_cast_p (use_stmt)) > + continue; > + > + tree lhs = gimple_assign_lhs (use_stmt); > + > + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (lhs) > + || (gimple_assign_rhs1 (use_stmt) > + != gimple_assign_rhs1 (def_stmt)) > + || !types_compatible_p (TREE_TYPE (name), TREE_TYPE (lhs))) > + continue; > + > + bool use_dominates; > + if (gimple_bb (def_stmt) == gimple_bb (use_stmt)) > + { > + gimple_stmt_iterator gsi = gsi_for_stmt (use_stmt); > + while (!gsi_end_p (gsi) && gsi_stmt (gsi) != def_stmt) > + gsi_next (&gsi); > + use_dominates = !gsi_end_p (gsi); > + } > + else if (dominated_by_p (CDI_DOMINATORS, gimple_bb (use_stmt), > + gimple_bb (def_stmt))) > + use_dominates = false; > + else if (dominated_by_p (CDI_DOMINATORS, gimple_bb (def_stmt), > + gimple_bb (use_stmt))) > + use_dominates = true; > + else > + continue; > + > + if (use_dominates) > + { > + std::swap (name, lhs); > + std::swap (def_stmt, use_stmt); > + } > + } > + > + /* Now go through all uses of SRC again, replacing the equivalent > + dominated conversions. We may replace defs that were not > + dominated by the then-prevailing defs when we first visited > + them. */ > + FOR_EACH_IMM_USE_STMT (use_stmt, use_iter, src) > + { > + if (use_stmt == def_stmt > + || !gimple_assign_cast_p (use_stmt)) > + continue; > + > + tree lhs = gimple_assign_lhs (use_stmt); > + > + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (lhs) > + || (gimple_assign_rhs1 (use_stmt) > + != gimple_assign_rhs1 (def_stmt)) > + || !types_compatible_p (TREE_TYPE (name), TREE_TYPE (lhs))) > + continue; > + > + if (gimple_bb (def_stmt) == gimple_bb (use_stmt) > + || dominated_by_p (CDI_DOMINATORS, gimple_bb (use_stmt), > + gimple_bb (def_stmt))) > + { > + sincos_stats.conv_removed++; > + > + gimple_stmt_iterator gsi = gsi_for_stmt (use_stmt); > + replace_uses_by (lhs, name); > + gsi_remove (&gsi, true); > + } > + } > + > + return name; > +} > + > /* Records an occurrence at statement USE_STMT in the vector of trees > STMTS if it is dominated by *TOP_BB or dominates it or this basic block > is not yet initialized. Returns true if the occurrence was pushed on > @@ -1147,6 +1248,8 @@ execute_cse_sincos_1 (tree name) > int i; > bool cfg_changed = false; > > + name = execute_cse_conv_1 (name); > + > FOR_EACH_IMM_USE_STMT (use_stmt, use_iter, name) > { > if (gimple_code (use_stmt) != GIMPLE_CALL > @@ -1181,7 +1284,7 @@ execute_cse_sincos_1 (tree name) > and, in subsequent rounds, that the built_in type is the same > type, or a compatible type. */ > if (type != t && !types_compatible_p (type, t)) > - return false; > + RETURN_FROM_IMM_USE_STMT (use_iter, false); > } > if (seen_cos + seen_sin + seen_cexpi <= 1) > return false; > @@ -2296,6 +2399,8 @@ pass_cse_sincos::execute (function *fun) > > statistics_counter_event (fun, "sincos statements inserted", > sincos_stats.inserted); > + statistics_counter_event (fun, "conv statements removed", > + sincos_stats.conv_removed); > > return cfg_changed ? TODO_cleanup_cfg : 0; > } > > > > > -- > Alexandre Oliva, happy hacker > https://FSFLA.org/blogs/lxo/ > Free Software Activist > GNU Toolchain Engineer