Bug 117 - RISCV FCLASS instruction needed
Summary: RISCV FCLASS instruction needed
Status: PAYMENTPENDING FIXED
Alias: None
Product: Libre-SOC's first SoC
Classification: Unclassified
Component: ALU (including IEEE754 16/32/64-bit FPU) (show other bugs)
Version: unspecified
Hardware: Other Linux
: --- enhancement
Assignee: Luke Kenneth Casson Leighton
URL:
Depends on:
Blocks: 48
  Show dependency treegraph
 
Reported: 2019-07-26 22:18 BST by Luke Kenneth Casson Leighton
Modified: 2021-04-20 14:48 BST (History)
2 users (show)

See Also:
NLnet milestone: NLnet.2019.02
total budget (EUR) for completion of task and all subtasks: 125
budget (EUR) for this task, excluding subtasks' budget: 125
parent task for budget allocation: 48
child tasks for budget allocation:
The table of payments (in EUR) for this task; TOML format:
"lkcl"={amount=125, paid=2019-07-30}


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Kenneth Casson Leighton 2019-07-26 22:18:04 BST
return bits as follows:

rd bit | Meaning
0      | rs1 is −∞.
1      | rs1 is a negative normal number.
2      | rs1 is a negative subnormal number.
3      | rs1 is −0.
4      | rs1 is +0.
5      | rs1 is a positive subnormal number.
6      | rs1 is a positive normal number.
7      | rs1 is +∞.
8      | rs1 is a signaling NaN.
9      | rs1 is a quiet NaN.
only one bit set.
all other rd bits are cleared.
no fp exceptions.
Comment 1 Jacob Lifshay 2019-07-26 22:51:55 BST
(In reply to Luke Kenneth Casson Leighton from comment #0)
> FCLASS detects a "boxed" number type (must include FP16).

no it doesn't, it just treats it as a NaN. if a fp register holds a boxed f32, fclass.d returns that it's a quiet nan, since the exponent is all ones, the mantissa is non-zero and the quiet bit is set (msb of mantissa).

boxed fp32: 0xFFFF_FFFF_XXXX_XXXX
exponent field: 0x7FF0_0000_0000_0000
quiet bit: 0x0008_0000_0000_0000
Comment 2 Luke Kenneth Casson Leighton 2019-07-26 22:55:42 BST
Ok cool can you update comment #1 to outline clearly what's needed? cf the section in the RV spec, am using phone peck at the moment :)
Comment 3 Luke Kenneth Casson Leighton 2019-07-26 23:01:06 BST
Found section.

rd bit Meaning 
0 rs1 is -∞. 
1 rs1 is a negative normal number. 
2 rs1 is a negative subnormal number. 
3 rs1 is -0. 
4 rs1 is +0. 
5 rs1 is a positive subnormal number. 
6 rs1 is a positive normal number. 
7 rs1 is +∞. 
8 rs1 is a signaling NaN. 
9 rs1 is a quiet NaN.
Table 8.5
Comment 4 Jacob Lifshay 2019-07-26 23:02:37 BST
(In reply to Luke Kenneth Casson Leighton from comment #2)
> Ok cool can you update comment #1 to outline clearly what's needed? cf the
> section in the RV spec, am using phone peck at the moment :)
on my phone too
Comment 5 Luke Kenneth Casson Leighton 2019-07-26 23:07:00 BST
https://stackoverflow.com/questions/8341395/what-is-a-subnormal-floating-point-number

N126 already exists which if exp is -126 (N126), that means num is subnormal.

http://git.libre-riscv.org/?p=ieee754fpu.git;a=blob;f=src/ieee754/fpcommon/fpbase.py;h=5f7df717e374719c0b8b1ec1c2eee13feda427f0;hb=refs/heads/master#l321

Pffh. It should be dirt simple, this one. FPBase has all the tests already, and they are used in specialcases a *lot*.
Comment 6 Luke Kenneth Casson Leighton 2019-07-28 11:15:13 BST
ok added fclass test, including adding some extra functions to
FPFormat, jacob can you verify the logic there, i had to add
not just the hardware-check, there's no softfloat-3 "fclass"
function so had to write that as well: that's unverified-check
against unverified-check.
Comment 7 Jacob Lifshay 2019-07-28 12:08:18 BST
(In reply to Luke Kenneth Casson Leighton from comment #6)
> ok added fclass test, including adding some extra functions to
> FPFormat, jacob can you verify the logic there, i had to add
> not just the hardware-check, there's no softfloat-3 "fclass"
> function so had to write that as well: that's unverified-check
> against unverified-check.

FPFormat.is_nan is either misimplemented or misnamed/misdocumented. as written, it detects only quiet NaNs. to detect any NaN, the exponent has to be the max value and the mantissa non-zero.

the documentation on FPFormat.mantissa_mask and FPFormat.is_zero is incorrect.

I would rename get_mantissa to get_mantissa_field, since it seems reasonable to add a get_mantissa_value function that includes the implicit 1 bit (except on denormals/zero -- implicit 0 bit for those cases) since that is the mathematical mantissa value, not the version missing the implicit bit.

FPFormat.get_sign should be named get_sign_field and the docs adjusted, since I would expect get_sign to return 1 or -1, not 0 or 1.

is_subnormal needs a ')' in the doc string

I think I probably messed up this one: the doc string for mantissa_mask says exponent field

not that it matters much: FPFormat.is_nan_signalling: spell using one l, since that's how it's spelled in IEEE 754

handy addition: add a get_exponent_field that returns the bits without subtracting the bias and implement in terms of that, just compare get_exponent_field with exponent_(inf_nan|denormal_zero) without needing emax or e_sub, since that saves a subtraction and matches the rest of the class.

maybe add a exponent_mask property, like mantissa_mask, then implement get_exponent_field as:
def get_exponent_field(self, x):
    """ Returns the biased exponent of its input number, x. """
    return (x & self.exponent_mask) >> self.m_width

other than all that, FPFormat appears to be correct
Comment 8 Jacob Lifshay 2019-07-28 12:10:57 BST
maybe use is_signaling_nan instead of is_nan_signalling, since it works just fine on non-NaN values.
Comment 9 Jacob Lifshay 2019-07-28 12:16:55 BST
for consistency's sake, we should probably do a project-wide refactor switching exponent naming to exponent_field/biased_exponent and exponent_value/unbiased_exponent, where biased means that it's the raw bits from the fp number, and unbiased means the bias was subtracted to give the mathematical exponent.

(edit: cross-ref bug #121)
Comment 10 Luke Kenneth Casson Leighton 2019-07-28 12:42:45 BST
(In reply to Jacob Lifshay from comment #7)
> (In reply to Luke Kenneth Casson Leighton from comment #6)
> > ok added fclass test, including adding some extra functions to
> > FPFormat, jacob can you verify the logic there, i had to add
> > not just the hardware-check, there's no softfloat-3 "fclass"
> > function so had to write that as well: that's unverified-check
> > against unverified-check.
> 
> FPFormat.is_nan is either misimplemented or misnamed/misdocumented. as
> written, it detects only quiet NaNs. to detect any NaN, the exponent has to
> be the max value and the mantissa non-zero.

done.  created is_quiet_nan as well

> 
> the documentation on FPFormat.mantissa_mask and FPFormat.is_zero is
> incorrect.

sorted.

> I would rename get_mantissa to get_mantissa_field, 

agreed, done.

> since it seems reasonable
> to add a get_mantissa_value function that includes the implicit 1 bit
> (except on denormals/zero -- implicit 0 bit for those cases) since that is
> the mathematical mantissa value, not the version missing the implicit bit.

good idea - leaving it for now though
 
> FPFormat.get_sign should be named get_sign_field and the docs adjusted,
> since I would expect get_sign to return 1 or -1, not 0 or 1.

yep, done

> is_subnormal needs a ')' in the doc string

done.
 
> I think I probably messed up this one: the doc string for mantissa_mask says
> exponent field

corrected.
 
> not that it matters much: FPFormat.is_nan_signalling: spell using one l,
> since that's how it's spelled in IEEE 754

done.
 
> handy addition: add a get_exponent_field that returns the bits without
> subtracting the bias and implement in terms of that, 

done.

> just compare
> get_exponent_field with exponent_(inf_nan|denormal_zero) without needing
> emax or e_sub, since that saves a subtraction and matches the rest of the
> class.

mmm... not keen on that: the hardware version always subtracts the exponent
bias (everywhere), which means that FPFormat is the odd one out.
 
> maybe add a exponent_mask property, like mantissa_mask, then implement
> get_exponent_field as:
> def get_exponent_field(self, x):
>     """ Returns the biased exponent of its input number, x. """
>     return (x & self.exponent_mask) >> self.m_width

yep done, and used it to create get_exponent (which _does_ subtract the bias)

> other than all that, FPFormat appears to be correct

cool.

does test_fpclass_pipe.py look reasonable, i.e. conformant to the FCLASS
spec?
Comment 11 Jacob Lifshay 2019-07-28 13:25:42 BST
(In reply to Luke Kenneth Casson Leighton from comment #10)
> (In reply to Jacob Lifshay from comment #7)
> > just compare
> > get_exponent_field with exponent_(inf_nan|denormal_zero) without needing
> > emax or e_sub, since that saves a subtraction and matches the rest of the
> > class.
> 
> mmm... not keen on that: the hardware version always subtracts the exponent
> bias (everywhere), which means that FPFormat is the odd one out.
ok.

> does test_fpclass_pipe.py look reasonable, i.e. conformant to the FCLASS
> spec?

yes, though the conditions are quite redundant as written and it needs to be updated to match FPFormat.
Comment 12 Luke Kenneth Casson Leighton 2019-07-28 13:30:51 BST
(In reply to Jacob Lifshay from comment #11)

> > does test_fpclass_pipe.py look reasonable, i.e. conformant to the FCLASS
> > spec?
> 
> yes, though the conditions are quite redundant as written and it needs to be
> updated to match FPFormat.

i originally wrote FPClassMod in that format, from the spec, in an "i can
read and understand this according to the RISCV spec" way.

i then looked at the rocket-chip chisel3 code, and went "i get that this
is optimal as far as hardware is concerned, so let's go with it... however
i simply can't understand easily why it works".

i'm more concerned, in the *testing*, about readability and understandability
(not about "performance" of the testing code).

however in the hardware, gate count is clearly more important, even at the
expense of readability.
Comment 13 Luke Kenneth Casson Leighton 2019-07-30 11:42:42 BST
https://git.libre-riscv.org/?p=ieee754fpu.git;a=commitdiff;h=78cbe8c5131a84426a3cad4b0b3ed4ab7da49844

added coverage test (nan, zero, subnormal, random), works great.