Re: [patches] Re: [PATCH 06/12] RISC-V: Generic <math.h> and soft-fp Routines

[Palmer Dabbelt <palmer at dabbelt dot com>]

On Wed, 14 Jun 2017 13:59:48 PDT (-0700), joseph@codesourcery.com wrote:
> On Wed, 14 Jun 2017, Palmer Dabbelt wrote:
>
>> diff --git a/sysdeps/riscv/bits/fenv.h b/sysdeps/riscv/bits/fenv.h
>
>> +enum
>> +  {
>> +    FE_INEXACT = 0x01,
>> +#define FE_INEXACT	FE_INEXACT
>> +    FE_UNDERFLOW = 0x02,
>> +#define FE_UNDERFLOW	FE_UNDERFLOW
>> +    FE_OVERFLOW = 0x04,
>> +#define FE_OVERFLOW	FE_OVERFLOW
>> +    FE_DIVBYZERO = 0x08,
>> +#define FE_DIVBYZERO	FE_DIVBYZERO
>> +    FE_INVALID = 0x10,
>> +#define FE_INVALID	FE_INVALID
>> +  };
>
> This clearly hasn't passed the glibc testsuite.  See how other bits/fenv.h
> headers now define macros to make them usable in #if (which the conform/
> tests test for).

Yep -- I went ahead and fixed this failure while cleaning up the test suite..
Thanks for the heads up!

>> diff --git a/sysdeps/riscv/bits/mathdef.h b/sysdeps/riscv/bits/mathdef.h
>
> You shouldn't need this file any more.
>
>> diff --git a/sysdeps/riscv/bits/mathinline.h b/sysdeps/riscv/bits/mathinline.h
>
>> +/* Test for negative number.  Used in the signbit() macro.  */
>> +__MATH_INLINE int
>> +__NTH (__signbitf (float __x))
>> +{
>> +  return __builtin_signbitf (__x);
>> +}
>> +__MATH_INLINE int
>> +__NTH (__signbit (double __x))
>> +{
>> +  return __builtin_signbit (__x);
>> +}
>
> Is this really of any use now, given that signbit uses __builtin_signbit
> directly and that the signbit macro, rather than __signbit etc. functions,
> is used for internal calls within glibc?
>
>> +/* Leave it to the compiler to optimize these if __NO_MATH_ERRNO__.  */
>> +# if defined __riscv_flen && !defined __NO_MATH_ERRNO__
>> +
>> +#  if __riscv_xlen >= 64
>> +
>> +__MATH_INLINE long long int
>> +__NTH (llrintf (float __x))
>> +{
>> +  long long int __res;
>> +  __asm__ __volatile__ ("fcvt.l.s %0, %1" : "=r" (__res) : "f" (__x));
>> +  return __res;
>
> This all looks rather like making inline functions replicate the bugs of
> out-of-line functions (bug 6797, 6798), i.e. a bad idea.
>
>> diff --git a/sysdeps/riscv/ieee754.h b/sysdeps/riscv/ieee754.h
>
> My reading of the ABI is that long double is 128-bit unconditionally for
> RISC-V.  Thus, you shouldn't need this header; the default
> sysdeps/ieee754/ldbl-128/ieee754.h should suffice.
>
> To pass the math/ tests for soft-float, I'd expect you to need to have
> s_fma.c and s_fmaf.c that include the soft-fp versions, as on various
> other architectures.  (You also need a libm-test-ulps file or files, which
> appear to be missing from this patch series, to reflect the actual ulps
> seen on RISC-V.)

Thanks for the feedback.  Andrew went through and fixed a bunch of these up.
I'm triaging the test suite to see where we stand, and I'll make sure we
generate the ULPs files.  I believe we need three (soft, single, and double).