Re: [PATCH v8 17/35] Hexagon (target/hexagon/fma_emu.[ch]) utility functions

From: Richard Henderson <richard.henderson@linaro.org>
To: Taylor Simpson <tsimpson@quicinc.com>, qemu-devel@nongnu.org
Cc: ale@rev.ng, alex.bennee@linaro.org, philmd@redhat.com,
	laurent@vivier.eu, bcain@quicinc.com
Subject: Re: [PATCH v8 17/35] Hexagon (target/hexagon/fma_emu.[ch]) utility functions
Date: Sun, 14 Feb 2021 15:14:59 -0800	[thread overview]
Message-ID: <cd233aee-e185-e9e5-2ad0-0493c93f1ad5@linaro.org> (raw)
In-Reply-To: <1612763186-18161-18-git-send-email-tsimpson@quicinc.com>

On 2/7/21 9:46 PM, Taylor Simpson wrote:
> +#define DF_NAN         0xffffffffffffffffULL
> +#define DF_INF         0x7ff0000000000000ULL
> +#define DF_MINUS_INF   0xfff0000000000000ULL
> +#define DF_MAXF        0x7fefffffffffffffULL
> +#define DF_MINUS_MAXF  0xffefffffffffffffULL
...
> +#define SF_INF         0x7f800000
> +#define SF_MINUS_INF   0xff800000
> +#define SF_MAXF        0x7f7fffff
> +#define SF_MINUS_MAXF  0xff7fffff

Redundant with softfloat.  Is the default nan really -1?  I suppose then that
hexagon does not distinguish QNaN from SNaN?

You'll want to patch fpu/softfloat-specialize.c.inc for both of these choices:
no_signaling_nans and parts_default_nan.

> +typedef union {
> +    double f;
> +    uint64_t i;
> +    struct {
> +        uint64_t mant:52;
> +        uint64_t exp:11;
> +        uint64_t sign:1;
> +    };
> +} Double;

You cannot use a union with bitfields portably.  This will fail on a big-endian
host.  Anyway, extracting these bits of a float are already present via softfloat.

> +static inline Int128 int128_mul_6464(uint64_t ai, uint64_t bi)
> +{
> +    Int128 a, b;
> +    uint64_t pp0, pp1a, pp1b, pp1s, pp2;
> +
> +    a = int128_make64(ai);
> +    b = int128_make64(bi);
> +    pp0 = (uint64_t)int128_getw0(a) * (uint64_t)int128_getw0(b);
> +    pp1a = (uint64_t)int128_getw1(a) * (uint64_t)int128_getw0(b);
> +    pp1b = (uint64_t)int128_getw1(b) * (uint64_t)int128_getw0(a);
> +    pp2 = (uint64_t)int128_getw1(a) * (uint64_t)int128_getw1(b);
> +
> +    pp1s = pp1a + pp1b;
> +    if ((pp1s < pp1a) || (pp1s < pp1b)) {
> +        pp2 += (1ULL << 32);
> +    }
> +    uint64_t ret_low = pp0 + (pp1s << 32);
> +    if ((ret_low < pp0) || (ret_low < (pp1s << 32))) {
> +        pp2 += 1;
> +    }
> +
> +    return int128_make128(ret_low, pp2 + (pp1s >> 32));
> +}

This is duplicating code from include/fpu/softfloat-macros.h, except for the
wrapping to Int128.  That said, I don't think you should actually need this,
or, frankly, the vast majority of the rest of your fp code.

> +typedef struct {
> +    Int128 mant;
> +    int32_t exp;
> +    uint8_t sign;
> +    uint8_t guard;
> +    uint8_t round;
> +    uint8_t sticky;
> +} Accum;

Um.. what?  Why in the world would you split the 3 guard bits away from the
rest of mant?

> +/* Return an infinity with requested sign */
> +static inline float64 infinite_float64(uint8_t sign)
> +{
> +    if (sign) {
> +        return make_float64(DF_MINUS_INF);
> +    } else {
> +        return make_float64(DF_INF);
> +    }
> +}

Surely just float64_set_sign(float64_infinity, sign).

> +static bool is_inf_prod(float64 a, float64 b)
> +{
> +    return ((float64_is_infinity(a) && float64_is_infinity(b)) ||
> +            (float64_is_infinity(a) && is_finite(b) && (!float64_is_zero(b))) ||

This is always false, because is_finite excludes infinity.

> +float32 internal_fmafx(float32 a, float32 b, float32 c, int scale,
> +                       float_status *fp_status)
> +{

Right.  So, best I can figure, all of this support code that's re-implementing
softfloat stuff is just so you can add "int scale" to floatXX_muladd.

Currently, we have a single bit to affect scaling of muladd (2**-1), for Arm.
It would be easy to adjust the softfloat implementation to handle arbitrary
scaling.

I would vastly prefer to do that than do this.

> +float64 internal_mpyhh(float64 a, float64 b,
> +                      unsigned long long int accumulated,
> +                      float_status *fp_status)

I really don't understand what this is doing.  Sadly, the hexagon manual
doesn't bother to define some of its pseudocode functions, and this (dfmpyhh)
is one of them.

r~