Pyrtl floating point library

[gaborszita]

No description provided.

[codecov]

Codecov Report

❌ Patch coverage is 98.89503% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.9%. Comparing base (8c706f6) to head (e601040).
⚠️ Report is 5 commits behind head on development.

Files with missing lines	Patch %	Lines
pyrtl/rtllib/pyrtlfloat/floatoperations.py	91.9%	4 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##           development    #475     +/-   ##
=============================================
+ Coverage         91.0%   91.9%   +0.9%     
=============================================
  Files               25      31      +6     
  Lines             7091    7452    +361     
=============================================
+ Hits              6450    6845    +395     
+ Misses             641     607     -34     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fdxmw]

Thank you for this contribution! Here are some initial comments

[fdxmw]

Generally, classes with only @staticmethods probably shouldn't be classes :) It looks like this class can be removed, and all the static methods can be ordinary functions? Same comment for FloatUtils and MultiplicationHelper

[fdxmw]

Consider using wire_struct to simplify these kinds of concat/slice patterns. With a wire_struct, you wouldn't need this line, and later lines would instead refer to raw_result.guard.

Also see the wire_struct example.

[fdxmw]

Similarly, wire_struct can remove the need for these tricky bit offset calculations

[gaborszita]

I was trying to do this, but the problem is that the number of mantissa and exponent bits differs based on fp_prop, so we would need to set the bitwidths of the slices in the wire_struct dynamically at runtime. Is it possible to do this with wire_struct?

[fdxmw]

It is possible with some trickery, but it's probably overkill in this case, since you only need to select between a few well-known bit layouts (the user can't define an arbitrary custom floating point format). I think you could define a set of wire_structs that share the same interface, something like:

@pyrtl.wire_struct
class Float16
    sign: 1
    exponent: 5
    fraction: 10

@pyrtl.wire_struct
class Float32
    sign: 1
    exponent: 8
    fraction: 23

So the idea is that these objects share the same interface, so it's easier to write code that works with any of these types. For example, if you want to compare sign bits, you'd write a.sign == b.sign, and that would work regardless of whether a or b are Float16 or Float32. If you care about types (I encourage you to! :), these are Unions like Float16 | Float32.

In case it's useful, here's the trickery, you can define a function that returns a dynamically-defined class:

import pyrtl

def define_my_struct(a_bits: int, b_bits: int):
    @pyrtl.wire_struct
    class MyInternalClass:
        a: a_bits
        b: b_bits

    return MyInternalClass

MyStruct = define_my_struct(a_bits=4, b_bits=8)

my_struct_instance = MyStruct(a=1, b=2)

print("a bitwidth", my_struct_instance.a.bitwidth)
print("b bitwidth", my_struct_instance.b.bitwidth)
print("total bitwidth", my_struct_instance.bitwidth)

$ uv run ...
a bitwidth 4
b bitwidth 8
total bitwidth 12

[fdxmw]

We'll need docstrings for all user-facing classes, methods, and functions. Docstrings for internal stuff would also be great; for example as someone new to this code it would really help to have an idea of what make_denormals_zero does conceptually.

[fdxmw]

It would really help to have some comments with brief definitions and references for important floating point concepts (NaN, inf, denormalized (how are these different?), guard, round, sticky, ...).

I'm not expecting you to teach me everything in comments, but it would really help to know what I should read if I want to understand how this works :)

[fdxmw]

I think you can omit the 0 here, also on line 119 below. Doing so would be more consistent with your code above, line 63 for example.

[fdxmw]

Capitalization of NaN is inconsistent here, it's nan on the left hand side and NaN on the right hand side. I'd rename is_NaN to is_nan since we pretty consistently use lowercase with underscores for method and function names.

[fdxmw]

We'll need a better story for how this is tested :) A few things to consider:

Every PyRTL developer will need to run these tests for the indefinite future, so we really don't want the tests to run for too long. I'd say all continuously-run floating point tests should complete in a couple seconds or less. These tests can't be comprehensive, but we should still try to get as much value as we can out of them.

Try to think about the most interesting inputs (zeroes, inf, nan, ...), and which combinations of inputs and operations are worth testing. I would avoid randomly generated tests because they are unlikely to cover all the interesting combinations with a reasonable number of test cases.

Think about how someone besides yourself would debug one of these test failures. It would help a lot to know what cases a particular test exercises (# Test addition with inf, etc). It also helps to know if the unexpected output is slightly wrong, or complete nonsense, which is pretty hard to tell from a raw bit pattern (see next point :)

Consider adding some helper functions that can convert float to and from these bit patterns. That would make it easier for someone to understand that the test checks that 1.0 + 2.0 == 3.0, for example.

[gaborszita]

Yep, testing needs more work, I'll add more rigorous tests and test edge cases. That's why I marked it as a draft PR because it is not finished yet.

[fdxmw]

This doesn't seem very useful, since it just passes all its arguments through to MultiplicationHelper.multiply? Seems better to point users directly to MultiplicationHelper.multiply. Same comments for the other methods below.

[gaborszita]

The reason I have this FloatOperations class and separate MultiplicationHelper and AddSubHelper classes is because the user is supposed to use FloatOperations, MultiplicationHelper and AddSubHelper are internal helpers and the user is not supposed to use them. I created these helpers so I can separate the multiplication and addition/subtraction logic into separate files.

[fdxmw]

I think that makes sense, but I'd still try to refactor, it would be nice to remove these functions that are just pass-through wrappers for another function. Maybe:

1. AddSubHelper.add could be an ordinary function instead of a @staticmethod
2. _add_sub.py could be renamed to add_sub.py
3. Non-public functions in add_sub.py could have an underscore prefix
4. __init__.py could import add from add_sub

I think that would still make it clear which parts of the interface are meant for public use, while removing a layer of indirection?

This kind of indirection tends to be annoying when debugging problems, because you have to jump through another hoop to find the code you're looking for. "Our princess is in another castle!"

[fdxmw]

All user-facing methods/functions should check (somewhere, not necessarily here) that the operand bitwidths are consistent with config

[fdxmw]

Sorry for the slow review! This code is making a lot more sense to me now, and I think it looks pretty good! I've only reviewed the main addition and subtraction code for now, I'll take a look at multiplication tomorrow. Here are some comments in the meantime.

Also I still stand by my earlier comments about trying to remove layers of indirection that don't do much work (FloatOperations, _BaseTypedFloatOperations) and trying to use WireStruct instead of calculating bit offsets :)

[fdxmw]

The words first and second here are a bit confusing because I can interpret it the other way, for example if I shift the bits one at a time, the first bit that gets shifted out is the least significant bit (smaller_operand_mantissa[0]), regardless of the shift_amount.

Maybe the first bit could be something like "the most significant bit to the right of the mantissa, after shifting"?

[fdxmw]

This is fine for now, but keep in mind that shift_right_logical creates a barrel shifter, which is pretty expensive, and we just made one on L75 above (and we'll make another on L95 below :). It should be possible to use one barrel shifter instead of three, by appending zero-valued GRS bits to the smaller_operand_mantissa before we shift it. That should work for the guard and round bits, but properly tracking the sticky bit would likely require changes to the barrel shifter.

[fdxmw]

I think an important insight here is:

1. Shifting the mantissa right by one divides the value by two.
2. Adding one to the exponent multiplies the value by two.
3. If we simultaneously do (1) and (2), we don't change the value, but it helps match the operand's exponents for easier math.

[fdxmw]

Naming seems a little inconsistent here, above _add_operands returns sum_exponent, while here _sub_operands returns sub_exponent. Seems like this should be called difference_exponent (addition results in a sum, subtraction results in a difference)?

[fdxmw]

It's pretty counterintuitive that rounding_mode determines whether you get inf or largest_finite_number on overflow. A reference would be really useful here

[fdxmw]

Seems like this initialization should be = None, since it always gets overwritten in the if statement below? (calling pyrtl.Const(1) creates a Const and adds it to the Block, but that Const is never used)

[fdxmw]

It doesn't seem useful to give the rounded results new names (raw_result_rounded_exponent, etc), since we don't seem to use the not-rounded results (raw_result_exponent, etc). If this instead updated raw_result_exponent and friends, I think we could remove the conditional on L287 below, since we can always assign final_result_exponent |= raw_result_exponent ?

Also this function is quite long and there are a lot of variables, so it would really help readers to clarify variable lifetimes (which variables are still in use), and how groups of variables are related. Some ways to do this include:

1. Avoid creating new variables when it makes sense to reuse an existing one (this suggestion)
2. Try to define fewer variables with more internal structure, rather than defining a flat layer of top-level variables (raw_result.exponent rather than raw_result_exponent) (an earlier suggestion)
3. Factor out more functions, which clearly defines the inputs and outputs for each chunk of code, and creates more local variable scopes
4. Explicitly del variables that aren't needed anymore

[fdxmw]

Could use a comment like "Otherwise no special cases apply: this is the common case" :)

[fdxmw]

This should be called _float_utils.py? (only one L in utilities :)

[fdxmw]

A reference that defines denormalized numbers would be very helpful. https://en.wikipedia.org/wiki/Subnormal_number seems good, but maybe you've found something better?

[fdxmw]

It's a little confusing that make_denormals_zero returns a WireVector while make_inf conditionally assigns its argument WireVectors. Consistency is clearer, so try to be consistent :)

I'd recommend just returning WireVectors in all these methods because the implementation is much easier to understand. Very few people know what happens when you do a |= assignment outside of a conditional_assignment block.

[fdxmw]

I left a similar comment on _add_sub.py, but I'd redefine operands here rather than defining a new operands_daz since we don't seem to use operands anymore after this point.

Similarly, this is a big function, so think about how someone new to this code will read it, and look for ways to help them navigate and understand what's important.

Don't forget that you have the Curse of Knowledge: you wrote all this code, so it already makes perfect sense to you :) To really test your code's readability, you have to show it to other people.

[fdxmw]

Consider grouping the operands into a tuple, for consistency with the multiplication implementation, and removing redundant code. Maybe it should be a sorted tuple, so the smaller operand is operands[0] and we wouldn't need more names to track smaller/larger?

[fdxmw]

Similarly, assert that bits fits in 16 bits

[fdxmw]

All these tests seem to use the same hardware, so it would help to factor this logic out to make it obvious that we're still using the same hardware, or to clearly point out any differences. Trying to determine this as a reader is not easy :) Some ideas:

1. Just define all the tests that use the same hardware in the same class. You can add big section comments like

   ############################
   # Additional rounding tests.

   to group the different kinds of tests for people reading the code. I recommend this approach.
2. Factor the hardware definition out into a function.
3. Move the hardware definition to a base class.

[fdxmw]

Same nit about consistency, I've seen at least denormal, denorms, and denormalized in this PR. Try to pick one and use it consistently :)

[fdxmw]

I'm sure you've run into this, but think about how someone would debug one of these assertEqual failures. I think you'd get a message that says

{big decimal number} is not equal to {other big decimal number}"

which is not very helpful.

Most likely, the first thing you'd do to debug a failure is to call decode_float16 to split out the sign, exponent, and mantissa on the unexpected value. Since this is the most likely first debug step, it'd really help the next person that works on this (which could be you! :) to integrate decode_float16 into these checks.

Maybe we can define a assertFloat16Equal helper that internally calls decode_float16 and separately calls assertEqual on the sign, exponent, and mantissa?

[fdxmw]

Add links to https://en.wikipedia.org/wiki/NaN#Quiet_NaN and https://en.wikipedia.org/wiki/NaN#Encoding ?