Re: [PATCH v1 18/20] s390x/tcg: Implement VECTOR FP (MAXIMUM|MINIMUM)

From: Richard Henderson <richard.henderson@linaro.org>
To: David Hildenbrand <david@redhat.com>, qemu-devel@nongnu.org
Cc: qemu-s390x@nongnu.org, Cornelia Huck <cohuck@redhat.com>,
	Thomas Huth <thuth@redhat.com>
Subject: Re: [PATCH v1 18/20] s390x/tcg: Implement VECTOR FP (MAXIMUM|MINIMUM)
Date: Thu, 1 Oct 2020 11:49:33 -0500	[thread overview]
Message-ID: <a8050a4c-e613-7d80-f243-43098b0fb9b6@linaro.org> (raw)
In-Reply-To: <20200930145523.71087-19-david@redhat.com>

On 9/30/20 9:55 AM, David Hildenbrand wrote:
> +typedef enum S390MinMaxType {
> +    s390_minmax_java_math_min,
> +    s390_minmax_java_math_max,
> +    s390_minmax_c_macro_min,
> +    s390_minmax_c_macro_max,
> +    s390_minmax_fmin,
> +    s390_minmax_fmax,
> +    s390_minmax_cpp_alg_min,
> +    s390_minmax_cpp_alg_max,
> +} S390MinMaxType;

I think you'd do just as well making this enum match M6, so that no translation
is necessary.

> +
> +#define S390_MINMAX(BITS, TYPE)                                                \
> +static float##BITS TYPE##BITS(float##BITS a, float##BITS b, float_status *s)   \
> +{                                                                              \
> +    const bool zero_a = float##BITS##_is_infinity(a);                          \
> +    const bool zero_b = float##BITS##_is_infinity(b);                          \
> +    const bool inf_a = float##BITS##_is_infinity(a);                           \
> +    const bool inf_b = float##BITS##_is_infinity(b);                           \
> +    const bool nan_a = float##BITS##_is_infinity(a);                           \
> +    const bool nan_b = float##BITS##_is_infinity(b);                           \

Wrong lookups.

Note that you've already got float*_dcmask which you could use to help out
here.  You just need some named constants to help with the 12 different bits.

> +        switch (TYPE) {                                                        \
> +        case s390_minmax_java_math_min:                                        \
> +        case s390_minmax_java_math_max:                                        \

I think you should make TYPE a function parameter, and not replicate this
function N times, and so you also don't need

> +static vop32_3_fn const vfmax_fns32[16] = {
> +    [0] = float32_maxnum,
> +    [1] = s390_minmax_java_math_max32,
> +    [2] = s390_minmax_c_macro_max32,
> +    [3] = s390_minmax_cpp_alg_max32,
> +    [4] = s390_minmax_fmax32,
> +    [8] = float32_maxnummag,
> +    [9] = s390_minmax_java_math_max_abs32,
> +    [10] = s390_minmax_c_macro_max_abs32,
> +    [11] = s390_minmax_cpp_alg_max_abs32,
> +    [12] = s390_minmax_fmax_abs32,
> +};
> +
> +static vop64_3_fn const vfmax_fns64[16] = {
> +    [0] = float64_maxnum,
> +    [1] = s390_minmax_java_math_max64,
> +    [2] = s390_minmax_c_macro_max64,
> +    [3] = s390_minmax_cpp_alg_max64,
> +    [4] = s390_minmax_fmax64,
> +    [8] = float64_maxnummag,
> +    [9] = s390_minmax_java_math_max_abs64,
> +    [10] = s390_minmax_c_macro_max_abs64,
> +    [11] = s390_minmax_cpp_alg_max_abs64,
> +    [12] = s390_minmax_fmax_abs64,
> +};
> +
> +static vop128_3_fn const vfmax_fns128[16] = {
> +    [0] = float128_maxnum,
> +    [1] = s390_minmax_java_math_max128,
> +    [2] = s390_minmax_c_macro_max128,
> +    [3] = s390_minmax_cpp_alg_max128,
> +    [4] = s390_minmax_fmax128,
> +    [8] = float128_maxnummag,
> +    [9] = s390_minmax_java_math_max_abs128,
> +    [10] = s390_minmax_c_macro_max_abs128,
> +    [11] = s390_minmax_cpp_alg_max_abs128,
> +    [12] = s390_minmax_fmax_abs128,
> +};

these tables.

I think that the bulk of your minmax could be done exclusively with dcmask, so
there could be a common non-macroized function that returns an indication of
whether A or B (possibly silenced) should be the result, or if we should use
one of your two compare cases.

BTW, for your two "simple comparison" cases, have we eliminated all of the
special cases?  Could we in fact be calling floatN_min/max instead of
floatN_le_quiet?

r~