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

[Darius Rad <darius at bluespec dot com>]

On 01/01/2018 12:08 PM, Joseph Myers wrote:
> On Tue, 26 Dec 2017, Palmer Dabbelt wrote:
> 
>> +long
>> +__lrint (double x)
>> +{
>> +  long res;
> 
> long int, as in the previous patch.
> 

Indeed, and I will fix.

>> +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?
> 

You're correct, llrintf should be RV64 only.  I will remove it.

>> +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.
> 

Thanks.  I'll update to use int64_t/int32_t as appropriate for the assembly.

> (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).
> 

Thanks, good to know for the future.

>> +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).
> 

Yes, thanks.  I'll fix.

>> +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).
>