FPU rounding module #728

Durchbruchswagen · 2024-09-10T00:02:14Z

Implementation of the rounding module for FPU.

This module excepts that input is in normalized form (unless the number is subnormal) and, in accordance with RISC-V specification, detects tininess after rounding. The flag that allows generation of a rounding module that assumes that input was already rounded is to be used by modules that are able to perform both their operation and rounding at the same time (adder/subtractor, for example). In that case, this module only checks for errors and special cases (one of the inputs was inf/nan).

coreblocks/func_blocks/fu/fpu/fpu_rounding_module.py

piotro888 · 2024-09-24T16:17:40Z

coreblocks/func_blocks/fu/fpu/fpu_rounding_module.py

+        ]
+
+
+class FPUrounding(Component):


Docstring would be helpful

Done, added docstring

piotro888 · 2024-09-24T16:27:25Z

coreblocks/func_blocks/fu/fpu/fpu_rounding_module.py

+        self.rtval = {}
+        self.max_exp = C(
+            2 ** (self.fpu_rounding_params.fpu_params.exp_width) - 1,
+            unsigned(self.fpu_rounding_params.fpu_params.exp_width),
+        )
+        self.max_normal_exp = C(
+            2 ** (self.fpu_rounding_params.fpu_params.exp_width) - 2,
+            unsigned(self.fpu_rounding_params.fpu_params.exp_width),
+        )
+        self.quiet_nan = C(
+            2 ** (self.fpu_rounding_params.fpu_params.sig_width - 1),
+            unsigned(self.fpu_rounding_params.fpu_params.sig_width),
+        )
+        self.max_sig = C(
+            2 ** (self.fpu_rounding_params.fpu_params.sig_width) - 1,
+            unsigned(self.fpu_rounding_params.fpu_params.sig_width),
+        )
+        self.add_one = Signal()
+        self.inc_rtnte = Signal()
+        self.inc_rtnta = Signal()
+        self.inc_rtpi = Signal()
+        self.inc_rtmi = Signal()
+
+        self.rounded_sig = Signal(self.fpu_rounding_params.fpu_params.sig_width + 1)
+        self.normalised_sig = Signal(self.fpu_rounding_params.fpu_params.sig_width)
+        self.rounded_exp = Signal(self.fpu_rounding_params.fpu_params.exp_width + 1)
+
+        self.final_guard_bit = Signal()
+        self.final_sticky_bit = Signal()
+
+        self.overflow = Signal()
+        self.underflow = Signal()
+        self.inexact = Signal()
+        self.tininess = Signal()
+        self.is_inf = Signal()
+        self.is_nan = Signal()
+        self.input_not_special = Signal()
+        self.rounded_inexact = Signal()
+
+        self.final_exp = Signal(self.fpu_rounding_params.fpu_params.exp_width)
+        self.final_sig = Signal(self.fpu_rounding_params.fpu_params.sig_width)
+        self.final_sign = Signal()


those are module internal signals/constants, there is no need to make them public. definitions could be placed in elaborate code and without self.

piotro888 · 2024-09-24T16:59:48Z

coreblocks/func_blocks/fu/fpu/fpu_rounding_module.py

+            ("sign", 1),
+            ("sig", fpu_params.sig_width),
+            ("exp", fpu_params.exp_width),
+            ("guard_bit", 1),


Did you mean round_bit?

Yes, my bad. After reading a few papers, it can get a little bit confusing, but most of the books and papers define this bit as a round bit.

coreblocks/func_blocks/fu/fpu/fpu_rounding_module.py

test/func_blocks/fu/test_fpu_rounding.py

coreblocks/func_blocks/fu/fpu/fpu_rounding_module.py

piotro888 · 2024-09-24T17:31:44Z

coreblocks/func_blocks/fu/fpu/fpu_rounding_module.py

+                with m.If((self.rounded_exp == 0) & (self.normalised_sig[-1] == 1)):
+                    m.d.comb += self.final_exp.eq(1)


Can you explain this condition?

This is the corner case for rounding subnormal numbers. Basically, in IEEE 754 standard, the 0 and 1 exponents represent the same exponent exp_min (1-bias if I remember correctly). The difference is that exp = 1 indicates that the number is normal (the implicit bit is equal to 1) and exp = 0 indicates that the number is in denormalized form (the implicit bit is equal to 0). The numbers with exp = 0 are called subnormal (except 0). There are 3 possible results of rounding for a subnormal number: zero, subnormal number or smallest normal number. This condition is for the last case. I basically check if exp == 0 and if the msb bit (the one that is implicit) is equal to one. In that case, I have to change the exponent to 1 to indicate that the number is now normal.

tilk · 2024-10-03T14:12:57Z

test/func_blocks/fu/test_fpu_rounding.py

+            input_values_dict["exp"] = help_values.max_exp
+            input_values_dict["sig"] = help_values.qnan
+            input_values_dict["invalid_operation"] = 1
+            input_values_dict["division_by_zero"] = 0
+
+            resp = yield from request_adapter.call(input_values_dict)
+
+            assert resp["sign"] == input_values_dict["sign"]
+            assert resp["exp"] == input_values_dict["exp"]
+            assert resp["sig"] == input_values_dict["sig"]
+            assert resp["errors"] == 1


There is a lot of needlessly duplicated code here, which would make maintenance harder. You should use a loop over a list of test cases.

Done, there are now two arrays, one with input data and one with expected results, that are looped.

piotro888

Sorry for late review

piotro888 · 2024-11-03T16:39:33Z

coreblocks/func_blocks/fu/fpu/fpu_error_module.py

+            ("sign", 1),
+            ("sig", fpu_params.sig_width),
+            ("exp", fpu_params.exp_width),
+            ("errors", 5),


Please use IntFlag enum as errors shape and when referencing them

piotro888 · 2024-11-03T16:58:18Z

coreblocks/func_blocks/fu/fpu/fpu_rounding_module.py

+        ]
+
+
+class FPUrounding(Elaboratable):


Suggested change

class FPUrounding(Elaboratable):

class FPURounding(Elaboratable):

typo?

Yes, my bad. Typo is now fixed.

piotro888 · 2024-11-03T17:02:21Z

coreblocks/func_blocks/fu/fpu/fpu_error_module.py

+    """
+
+    def __init__(self, *, fpu_params: FPUParams):
+        self.error_in_layout = [


Usage of input_inf as in input to previous stages is not obvious by name, comment about that here could be helpful for future.

Please resolve comment from previous review

Sorry for the late response. I pushed changes at around 3 a.m. and went to sleep with a plan to resolve comments next day, but by then you had already reviewed them. I was also thinking if maybe a better solution would be to write about this flag in some class for input layout for modules that perform arithmetic operations or something like that, but probably a small comment about why an error module needs it is warranted, so I added it.

I thought it was finished, sorry!

piotro888 · 2024-11-03T17:03:45Z

coreblocks/func_blocks/fu/fpu/fpu_common.py

+
+
+class FPUParams:
+    """FPU parameters


Same thing here with documentation: it would be helpful to add information that sig_width contains the implicit bit.

tilk · 2024-11-05T11:45:49Z

Waiting with merge until reviews are addressed.

piotro888 · 2024-11-10T12:50:44Z

coreblocks/func_blocks/fu/fpu/fpu_common.py

+class Errors(enum.IntFlag, shape=5):
+    INVALID_OPERATION = 0
+    DIVISION_BY_ZERO = 1
+    OVERFLOW = 2
+    UNDERFLOW = 3
+    INEXACT = 4


Suggested change

class Errors(enum.IntFlag, shape=5):

INVALID_OPERATION = 0

DIVISION_BY_ZERO = 1

OVERFLOW = 2

UNDERFLOW = 3

INEXACT = 4

class Errors(enum.IntFlag):

INVALID_OPERATION = auto()

DIVISION_BY_ZERO = auto()

OVERFLOW = auto()

UNDERFLOW = auto()

INEXACT = auto()

IntFlag is supposed to be used as bit flag value, and not bit index! (this is why shape was not working)

auto() can be used on IntFlags resolve to next power of two flags so INVALID_OPERATION=1 DIVISION_BY_ZERO=2 OVERFLOW=4
Shape should not be needed if used as flag.

My bad, I hadn't thought about assigning powers of 2 to enums. Changed Errors according to your suggestion

piotro888 · 2024-11-10T12:52:35Z

coreblocks/func_blocks/fu/fpu/fpu_error_module.py

+            ("sign", 1),
+            ("sig", fpu_params.sig_width),
+            ("exp", fpu_params.exp_width),
+            ("errors", Shape.cast(Errors)),


Suggested change

("errors", Shape.cast(Errors)),

("errors", Errors),

with change from previous comment it can be written without workaround

piotro888 · 2024-11-10T12:57:26Z

test/func_blocks/fu/test_fpu_error.py

+                # division by zero
+                {"sign": 0, "sig": 0, "exp": help_values.max_exp, "errors": 2},
+                # overflow but no round and sticky bits
+                {"sign": 0, "sig": 0, "exp": help_values.max_exp, "errors": 20},


Magic numbers from errors can be now replaced with simply ORing Error fields

piotro888 · 2024-11-10T13:17:12Z

coreblocks/func_blocks/fu/fpu/fpu_error_module.py

+            m.d.av_comb += final_errors[Errors.INVALID_OPERATION].eq(arg.invalid_operation)
+            m.d.av_comb += final_errors[Errors.DIVISION_BY_ZERO].eq(arg.division_by_zero)
+            m.d.av_comb += final_errors[Errors.OVERFLOW].eq(overflow)
+            m.d.av_comb += final_errors[Errors.UNDERFLOW].eq(underflow)
+            m.d.av_comb += final_errors[Errors.INEXACT].eq(inexact)


final_errors must be now constructed from ORing Error fields.
I think it can be done like that for now final_errors = Mux(arg.invalid_operation, Error.INVALID_OPERATION, 0) | .....

Extra note:
It is a common use case, and it cannot be done too cleanly, so we should probably later create some helper method in transactron library for that usage pattern or maybe suggest feature to amaranth enums?

piotro888 · 2024-11-10T13:18:05Z

coreblocks/func_blocks/fu/fpu/fpu_error_module.py

+    """
+
+    def __init__(self, *, fpu_params: FPUParams):
+        self.error_in_layout = [


Please resolve comment from previous review

piotro888 reviewed Sep 24, 2024

View reviewed changes

Durchbruchswagen requested a review from piotro888 October 1, 2024 20:34

tilk modified the milestones: Improve the core's performance, Implement floating point extensions Oct 3, 2024

tilk reviewed Oct 3, 2024

View reviewed changes

Durchbruchswagen requested a review from tilk October 7, 2024 14:10

tilk approved these changes Oct 22, 2024

View reviewed changes

Durchbruchswagen force-pushed the fpu_rounding branch from 14fa0fd to d519a1b Compare October 29, 2024 09:37

Durchbruchswagen added 12 commits October 29, 2024 10:49

Added FPU rounding module

1b8f7f4

Added FPU rounding module tests

b68f547

Fixed small spelling mistake

27a86d8

Fixed formatting in fpu_common

9c9f760

Review changes for fpu_rounding_module and fpu_common

6ba84c3

Review changes for test_fpu_rounding

d038fac

Small change to RoundingModes definition

cb09f65

Refactored tests

deb43e3

Split rounding module into rounding part and error checking part

d6aa8fe

Fixed docstring

c4ce728

Fixed formatting

2276d9b

Cleaned tests

d519a1b

piotro888 approved these changes Nov 3, 2024

View reviewed changes

Merge branch 'master' into fpu_rounding

c7e0fae

tilk added the enhancement New feature or request label Nov 5, 2024

Durchbruchswagen added 2 commits November 10, 2024 04:16

review changes

ae4553d

Fixed formatting

517c694

piotro888 reviewed Nov 10, 2024

View reviewed changes

Durchbruchswagen and others added 2 commits November 11, 2024 20:17

Review changes

57ba7bf

Merge branch 'master' into fpu_rounding

f43c939

Merge branch 'master' into fpu_rounding

1419ee4

tilk merged commit 34565a3 into kuznia-rdzeni:master Nov 12, 2024
13 checks passed

github-actions bot pushed a commit that referenced this pull request Nov 12, 2024

FPU rounding module (#728)

24dd71c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FPU rounding module #728

FPU rounding module #728

Durchbruchswagen commented Sep 10, 2024

piotro888 Sep 24, 2024

Durchbruchswagen Oct 1, 2024

piotro888 Sep 24, 2024

Durchbruchswagen Oct 1, 2024

piotro888 Sep 24, 2024

Durchbruchswagen Oct 1, 2024

piotro888 Sep 24, 2024

Durchbruchswagen Oct 1, 2024

tilk Oct 3, 2024

Durchbruchswagen Oct 7, 2024

piotro888 left a comment

piotro888 Nov 3, 2024

piotro888 Nov 3, 2024

Durchbruchswagen Nov 11, 2024

piotro888 Nov 3, 2024

piotro888 Nov 10, 2024

Durchbruchswagen Nov 11, 2024

piotro888 Nov 11, 2024

piotro888 Nov 3, 2024

Durchbruchswagen Nov 11, 2024

tilk commented Nov 5, 2024

piotro888 Nov 10, 2024

Durchbruchswagen Nov 11, 2024

piotro888 Nov 10, 2024

Durchbruchswagen Nov 11, 2024

piotro888 Nov 10, 2024

Durchbruchswagen Nov 11, 2024

piotro888 Nov 10, 2024

Durchbruchswagen Nov 11, 2024

piotro888 Nov 10, 2024

		with m.If((self.rounded_exp == 0) & (self.normalised_sig[-1] == 1)):
		m.d.comb += self.final_exp.eq(1)

	class FPUrounding(Elaboratable):
	class FPURounding(Elaboratable):

FPU rounding module #728

FPU rounding module #728

Conversation

Durchbruchswagen commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotro888 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tilk commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment