On Wed, Oct 7, 2020 at 7:15 PM Alexandre Oliva <ol...@adacore.com> wrote:
>
> On Oct 6, 2020, Richard Biener <richard.guent...@gmail.com> wrote:
>
> > So how about that mathfn_type helper instead of hard-wring this logic
> > in sincos()?
>
> Like this?
>
> Regstrapped on x86_64-linux-gnu. Ok to install?
OK with a minor nit, see below
> I'm a little unhappy with the duplication of the CASE_MATHFN* sequence,
> that ought to be kept in sync, , and considered turning that whole
> sequence into a #define used in both places, but that would bloat the
> patch further and make it less readable, so I figured I'd propose this
> one first. Please let me know if you agree this additional change would
> make it better.
Yeah, I guess so.
> take type from intrinsic in sincos pass
>
> From: Alexandre Oliva <ol...@adacore.com>
>
> This is a first step towards enabling the sincos optimization in Ada.
>
> The issue this patch solves is that sincos takes the type to be looked
> up with mathfn_built_in from variables or temporaries passed as
> arguments to SIN and COS intrinsics. In Ada, different float types
> may be used but, despite their representation equivalence, their
> distinctness causes the optimization to be skipped, because they are
> not the types that mathfn_built_in expects.
>
> This patch introduces a function that maps intrinsics to the type
> they're associated with, and uses that type, obtained from the
> intrinsics used in calls to be optimized, to look up the correspoding
> CEXPI intrinsic.
>
> For the sake of defensive programming, when using the type obtained
> from the intrinsic, it now checks that, if different types are found
> for the used argument, or for other calls that use it, that the types
> are interchangeable.
>
>
> for gcc/ChangeLog
>
> * builtins.c (mathfn_built_in_type): New.
> * builtins.h (mathfn_built_in_type): Declare.
> * tree-ssa-math-opts.c (execute_cse_sincos_1): Use it to
> obtain the type expected by the intrinsic.
> ---
> gcc/builtins.c | 147
> ++++++++++++++++++++++++++++++++++++++++++++++
> gcc/builtins.h | 1
> gcc/tree-ssa-math-opts.c | 17 ++++-
> 3 files changed, 162 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/builtins.c b/gcc/builtins.c
> index f91266e4..5649242 100644
> --- a/gcc/builtins.c
> +++ b/gcc/builtins.c
> @@ -2160,6 +2160,7 @@ mathfn_built_in_2 (tree type, combined_fn fn)
>
> switch (fn)
> {
> + /* Copied to mathfn_built_in_type, please keep in sync. */
> CASE_MATHFN (ACOS)
> CASE_MATHFN (ACOSH)
> CASE_MATHFN (ASIN)
> @@ -2278,6 +2279,10 @@ mathfn_built_in_2 (tree type, combined_fn fn)
> return END_BUILTINS;
> }
>
> +#undef CASE_MATHFN
> +#undef CASE_MATHFN_FLOATN
> +#undef CASE_MATHFN_REENT
> +
> /* Return mathematic function equivalent to FN but operating directly on
> TYPE,
> if available. If IMPLICIT_P is true use the implicit builtin declaration,
> otherwise use the explicit declaration. If we can't do the conversion,
> @@ -2313,6 +2318,148 @@ mathfn_built_in (tree type, enum built_in_function fn)
> return mathfn_built_in_1 (type, as_combined_fn (fn), /*implicit=*/ 1);
> }
>
> +/* Return the type associated with a built in function, i.e., the one
> + to be passed to mathfn_built_in to get the type-specific
> + function. */
> +
> +tree
> +mathfn_built_in_type (combined_fn fn)
> +{
> +#define CASE_MATHFN(MATHFN) \
> + case BUILT_IN_##MATHFN: \
> + return double_type_node; \
> + case BUILT_IN_##MATHFN##F: \
> + return float_type_node; \
> + case BUILT_IN_##MATHFN##L: \
> + return long_double_type_node;
> +
> +#define CASE_MATHFN_FLOATN(MATHFN) \
> + CASE_MATHFN(MATHFN) \
> + case BUILT_IN_##MATHFN##F16: \
> + return float16_type_node; \
> + case BUILT_IN_##MATHFN##F32: \
> + return float32_type_node; \
> + case BUILT_IN_##MATHFN##F64: \
> + return float64_type_node; \
> + case BUILT_IN_##MATHFN##F128: \
> + return float128_type_node; \
> + case BUILT_IN_##MATHFN##F32X: \
> + return float32x_type_node; \
> + case BUILT_IN_##MATHFN##F64X: \
> + return float64x_type_node; \
> + case BUILT_IN_##MATHFN##F128X: \
> + return float128x_type_node;
> +
> +/* Similar to above, but appends _R after any F/L suffix. */
> +#define CASE_MATHFN_REENT(MATHFN) \
> + case BUILT_IN_##MATHFN##_R: \
> + return double_type_node; \
> + case BUILT_IN_##MATHFN##F_R: \
> + return float_type_node; \
> + case BUILT_IN_##MATHFN##L_R: \
> + return long_double_type_node;
> +
> + switch (fn)
> + {
> + /* Copied from mathfn_built_in_2, please keep in sync. */
> + CASE_MATHFN (ACOS)
> + CASE_MATHFN (ACOSH)
> + CASE_MATHFN (ASIN)
> + CASE_MATHFN (ASINH)
> + CASE_MATHFN (ATAN)
> + CASE_MATHFN (ATAN2)
> + CASE_MATHFN (ATANH)
> + CASE_MATHFN (CBRT)
> + CASE_MATHFN_FLOATN (CEIL)
> + CASE_MATHFN (CEXPI)
> + CASE_MATHFN_FLOATN (COPYSIGN)
> + CASE_MATHFN (COS)
> + CASE_MATHFN (COSH)
> + CASE_MATHFN (DREM)
> + CASE_MATHFN (ERF)
> + CASE_MATHFN (ERFC)
> + CASE_MATHFN (EXP)
> + CASE_MATHFN (EXP10)
> + CASE_MATHFN (EXP2)
> + CASE_MATHFN (EXPM1)
> + CASE_MATHFN (FABS)
> + CASE_MATHFN (FDIM)
> + CASE_MATHFN_FLOATN (FLOOR)
> + CASE_MATHFN_FLOATN (FMA)
> + CASE_MATHFN_FLOATN (FMAX)
> + CASE_MATHFN_FLOATN (FMIN)
> + CASE_MATHFN (FMOD)
> + CASE_MATHFN (FREXP)
> + CASE_MATHFN (GAMMA)
> + CASE_MATHFN_REENT (GAMMA) /* GAMMA_R */
> + CASE_MATHFN (HUGE_VAL)
> + CASE_MATHFN (HYPOT)
> + CASE_MATHFN (ILOGB)
> + CASE_MATHFN (ICEIL)
> + CASE_MATHFN (IFLOOR)
> + CASE_MATHFN (INF)
> + CASE_MATHFN (IRINT)
> + CASE_MATHFN (IROUND)
> + CASE_MATHFN (ISINF)
> + CASE_MATHFN (J0)
> + CASE_MATHFN (J1)
> + CASE_MATHFN (JN)
> + CASE_MATHFN (LCEIL)
> + CASE_MATHFN (LDEXP)
> + CASE_MATHFN (LFLOOR)
> + CASE_MATHFN (LGAMMA)
> + CASE_MATHFN_REENT (LGAMMA) /* LGAMMA_R */
> + CASE_MATHFN (LLCEIL)
> + CASE_MATHFN (LLFLOOR)
> + CASE_MATHFN (LLRINT)
> + CASE_MATHFN (LLROUND)
> + CASE_MATHFN (LOG)
> + CASE_MATHFN (LOG10)
> + CASE_MATHFN (LOG1P)
> + CASE_MATHFN (LOG2)
> + CASE_MATHFN (LOGB)
> + CASE_MATHFN (LRINT)
> + CASE_MATHFN (LROUND)
> + CASE_MATHFN (MODF)
> + CASE_MATHFN (NAN)
> + CASE_MATHFN (NANS)
> + CASE_MATHFN_FLOATN (NEARBYINT)
> + CASE_MATHFN (NEXTAFTER)
> + CASE_MATHFN (NEXTTOWARD)
> + CASE_MATHFN (POW)
> + CASE_MATHFN (POWI)
> + CASE_MATHFN (POW10)
> + CASE_MATHFN (REMAINDER)
> + CASE_MATHFN (REMQUO)
> + CASE_MATHFN_FLOATN (RINT)
> + CASE_MATHFN_FLOATN (ROUND)
> + CASE_MATHFN_FLOATN (ROUNDEVEN)
> + CASE_MATHFN (SCALB)
> + CASE_MATHFN (SCALBLN)
> + CASE_MATHFN (SCALBN)
> + CASE_MATHFN (SIGNBIT)
> + CASE_MATHFN (SIGNIFICAND)
> + CASE_MATHFN (SIN)
> + CASE_MATHFN (SINCOS)
> + CASE_MATHFN (SINH)
> + CASE_MATHFN_FLOATN (SQRT)
> + CASE_MATHFN (TAN)
> + CASE_MATHFN (TANH)
> + CASE_MATHFN (TGAMMA)
> + CASE_MATHFN_FLOATN (TRUNC)
> + CASE_MATHFN (Y0)
> + CASE_MATHFN (Y1)
> + CASE_MATHFN (YN)
> +
> + default:
> + return NULL_TREE;
> + }
> +
> +#undef CASE_MATHFN
> +#undef CASE_MATHFN_FLOATN
> +#undef CASE_MATHFN_REENT
> +}
> +
> /* If BUILT_IN_NORMAL function FNDECL has an associated internal function,
> return its code, otherwise return IFN_LAST. Note that this function
> only tests whether the function is defined in internals.def, not whether
> diff --git a/gcc/builtins.h b/gcc/builtins.h
> index 504c618..72012a2 100644
> --- a/gcc/builtins.h
> +++ b/gcc/builtins.h
> @@ -109,6 +109,7 @@ extern void expand_builtin_setjmp_receiver (rtx);
> extern void expand_builtin_update_setjmp_buf (rtx);
> extern tree mathfn_built_in (tree, enum built_in_function fn);
> extern tree mathfn_built_in (tree, combined_fn);
> +extern tree mathfn_built_in_type (combined_fn);
> extern rtx builtin_strncpy_read_str (void *, HOST_WIDE_INT, scalar_int_mode);
> extern rtx builtin_memset_read_str (void *, HOST_WIDE_INT, scalar_int_mode);
> extern rtx expand_builtin_saveregs (void);
> diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c
> index 4927255..7504569 100644
> --- a/gcc/tree-ssa-math-opts.c
> +++ b/gcc/tree-ssa-math-opts.c
> @@ -1139,7 +1139,7 @@ execute_cse_sincos_1 (tree name)
> {
> gimple_stmt_iterator gsi;
> imm_use_iterator use_iter;
> - tree fndecl, res, type;
> + tree fndecl, res, type = NULL_TREE;
> gimple *def_stmt, *use_stmt, *stmt;
> int seen_cos = 0, seen_sin = 0, seen_cexpi = 0;
> auto_vec<gimple *> stmts;
> @@ -1147,7 +1147,6 @@ execute_cse_sincos_1 (tree name)
> int i;
> bool cfg_changed = false;
>
> - type = TREE_TYPE (name);
> FOR_EACH_IMM_USE_STMT (use_stmt, use_iter, name)
> {
> if (gimple_code (use_stmt) != GIMPLE_CALL
> @@ -1169,9 +1168,21 @@ execute_cse_sincos_1 (tree name)
> break;
>
> default:;
> + continue;
> }
> - }
>
> + tree t = mathfn_built_in_type (gimple_call_combined_fn (use_stmt));
> + if (!type)
> + {
> + type = t;
> + t = TREE_TYPE (name);
> + }
> + /* This checks that NAME has the right type in the first round,
> + and, in subsequent rounds, that the built_in type is the same
> + type. */
> + if (type != t && !tree_nop_conversion_p (type, t))
use !types_compatible_p (type, t) here if we don't want to go
with the stricter type == t (both yours and types_compatible_p
would treat long double and double the same in case they
are mapped to the same FP mode). types_compatible_p
is the appropriate GIMPLE predicate here.
Thanks,
Richard.
> + return false;
> + }
> if (seen_cos + seen_sin + seen_cexpi <= 1)
> return false;
>
>
>
>
> --
> Alexandre Oliva, happy hacker
> https://FSFLA.org/blogs/lxo/
> Free Software Activist
> GNU Toolchain Engineer
