FPU rounding module #728
Open
FPU rounding module #728
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
piotro888
reviewed
Sep 24, 2024
Comment on lines
+249
to
+255
|
|
||
| self.rtval["exp"] = self.final_exp | ||
| self.rtval["sig"] = self.final_sig | ||
| self.rtval["sign"] = self.final_sign | ||
| self.rtval["errors"] = self.final_errors | ||
|
|
||
| return self.rtval |
There was a problem hiding this comment.
You can use inline dictionary in return, like:
return {
"exp": self.final_exp,
...
Comment on lines
+67
to
+69
| class FPUrounding(Component): | ||
|
|
||
| fpu_rounding: FPURoundingSignature |
There was a problem hiding this comment.
Why is the Component used? Is the interface used aynwhere?
| def elaborate(self, platform): | ||
| m = TModule() | ||
|
|
||
| @def_method(m, self.rounding_request) |
There was a problem hiding this comment.
In this case, all of comb domains could be replaced with av_comb, to skip adding extra multiplexer (that enables the assignment only on rounding_request run condition).
| ] | ||
|
|
||
|
|
||
| class FPUrounding(Component): |
Comment on lines
+80
to
+121
| 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() |
There was a problem hiding this comment.
those are module internal signals/constants, there is no need to make them public. definitions could be placed in elaborate code and without self.
| ("sign", 1), | ||
| ("sig", fpu_params.sig_width), | ||
| ("exp", fpu_params.exp_width), | ||
| ("guard_bit", 1), |
|
|
||
| with m.Elif(self.overflow): | ||
|
|
||
| with m.If( |
There was a problem hiding this comment.
Amaranth switch/case could be used: with m.Switch(arg.rounding_mode)
Comment on lines
+335
to
+350
| yield from one_rounding_mode_test( | ||
| RoundingModes.ROUND_NEAREST_EVEN, | ||
| tie_to_even_inc_array, | ||
| 0, | ||
| help_values.max_exp, | ||
| 0, | ||
| help_values.max_exp, | ||
| request_adapter, | ||
| is_input_not_rounded, | ||
| ) | ||
| yield from one_rounding_mode_test( | ||
| RoundingModes.ROUND_NEAREST_AWAY, | ||
| tie_to_away_inc_array, | ||
| 0, | ||
| help_values.max_exp, | ||
| 0, |
There was a problem hiding this comment.
Tests can be parametrized with @parametrize_class, @parameterized.expand or similar
| ("exp", fpu_params.exp_width), | ||
| ("guard_bit", 1), | ||
| ("sticky_bit", 1), | ||
| ("rounding_mode", 3), |
There was a problem hiding this comment.
enums can be used as signal shapes
Suggested change
| ("rounding_mode", 3), | |
| ("rounding_mode", RoundingModes), |
Comment on lines
+237
to
+238
| with m.If((self.rounded_exp == 0) & (self.normalised_sig[-1] == 1)): | ||
| m.d.comb += self.final_exp.eq(1) |
There was a problem hiding this comment.
Can you explain this condition?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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).