Re: [PATCH v3 13/19] RISC-V: RV32D, RV64F, and RV64D Support

[Joseph Myers <joseph at codesourcery dot com>]

On Tue, 26 Dec 2017, Palmer Dabbelt wrote:

> +long
> +__lrint (double x)
> +{
> +  long res;

long int, as in the previous patch.

> +long
> +__lround (double x)
> +{
> +  long res;

Likewise.

> +long long
> +__llrintf (float x)
> +{
> +  long res;
> +  asm ("fcvt.w.s %0, %1" : "=r" (res) : "f" (x));
> +  return res;

Likewise (long long int), and I don't see how "long res;" here can be 
correct at all, since this is in an rv32 file, and float can represent 
integers outside the range of 32-bit long, which need to be correctly 
converted to 64-bit long long for this function.  Maybe llrintf should 
actually be rv64-only, since you need to convert to a 64-bit integer?

> +long
> +__lrintf (float x)
> +{
> +  long res;

Likewise.

> diff --git a/sysdeps/riscv/rv64/rvd/s_ceil.c b/sysdeps/riscv/rv64/rvd/s_ceil.c

> +      long i;

I would like to suggest, throughout the rv64 functions, where you are 
using long (as here) for a 64-bit internal temporary (as opposed to long 
int actually being part of the defined function API), you use int64_t 
instead of changing long -> long int - after all, the instructions you are 
using are defined to convert to/from 64-bit integers.  That way, the code 
would work unchanged if you support RV64 ILP32 in future, whereas if you 
use long int you need to change it again to support RV64 ILP32.

(More generally, if what you want about an integer type is a particular 
number of bits, using intN_t / uintN_t is encouraged.  That's definitely 
the case when using asm instructions as here that are defined to use 
integers with a given number of bits rather than linked to the C ABI.)

> +      long i;

Same in floor.

> +long long
> +__llrint (double x)
> +{
> +  long long res;

Use long long int.

> +long long
> +__llround (double x)
> +{
> +  long long res;

Use long long int.

> +long
> +__lrint (double x)
> +{
> +  long res;
> +  asm ("fcvt.l.d %0, %1" : "=r" (res) : "f" (x));

Use long int (but note a further change would be needed in future for RV64 
ILP32 where you want a conversion to 32-bit not 64-bit integer in the 
asm).

> +long
> +__lround (double x)
> +{
> +  long res;
> +  asm ("fcvt.l.d %0, %1, rmm" : "=r" (res) : "f" (x));

Likewise.

> +      long i;

int64_t in nearbyint as elsewhere.

> +      long i;

Likewise in rint.

> +      long i;

Likewise in round.

> +      long i;

Likewise in roundeven.

> +      long i;

Likewise in trunc.

> +long long
> +__llrintf (float x)
> +{
> +  long long res;

long long int.

> +#include <libm-alias-float.h>
> +#include <libm-alias-float.h>

You don't want to include the same header twice in a row (this is in 
llroundf).

> +long long
> +__llroundf (float x)
> +{
> +  long long res;

long long int.

> +long
> +__lrintf (float x)
> +{
> +  long res;
> +  asm ("fcvt.l.s %0, %1" : "=r" (res) : "f" (x));

long int (again, would need to change for ILP32 support).

-- 
Joseph S. Myers
joseph@codesourcery.com