-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Floating Point Square Root Module with Variable Odd Mantissa and No Rounding #188
base: main
Are you sure you want to change the base?
Conversation
Add a base class for floating point square root. Implement a basic floating point square root module. Mantissa is variable, but limited to odd numbers from 1 to 51. No rounding implemented. Implement a fixed point square root for values with three integral bits and up to 51 fractional bits. Add a fixed point square root method. Add tests for fixed-point and floating-point square root. Implements intel#171 Co-authored-by: James Farwell <james.c.farwell@intel.com> Co-authored-by: Curtis Anderson <curtis.anderson@intel.com> Signed-off-by: Stephen Weeks <stephen.weeks@intel.com> Signed-off-by: James Farwell <james.c.farwell@intel.com> Signed-off-by: Curtis Anderson <curtis.anderson@intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome work! Got me thinking about expanding for multi-cycle or pipelined versions :)
Rename 'width' member variable Add exception if signed numbers are attempted. Updated tests Reworked test cases into a single function to loop over all input values Aligned authors' names Remove unneedded library import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
additional changes pushed so far look good!
About to push more changes from Stephen, just need to run markdown lint first. |
Adding docs that pipelining is for future plumbing work Fixing sqrt output to be more sensical Removing magic number from random number test Fixing error signal for floating point sqrt logic Alligning names for floating_point_sqrt.dart Catching and properly reporting error on DeNorm Updating docs to mention we do not support DeNorms fixing fixed sqrt test
d4fcc35
to
0216f02
Compare
Alright, I have pushed changes from Stephen which address all of the current comments except the second one from @desmonddak (about avoiding injecting newlines in strings). |
Remove newlines and tabs (\n, \t)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
multi-line fix looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good to me! Great work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guys, this is beautiful work and I see you have used so many things we've asked for you to try. A great addition to ROHD-HCL and a solid coding example.
Description & Motivation
Sometimes we want to be able to do square root for floating points.
This is especially true for ML hardware.
This PR addresses that by adding a Floating Point Square Root module and extending
Fixed Point to have multiplication
Related Issue(s)
#171
Testing
Test cases were created for the Fixed Point Square Root and Floating Point.
Fixed Square Root tested:
Floating Point Tested:
Backwards-compatibility
No.
Documentation
Yes. This required changes to the Floating Point and Fixed Point docs within the HCL library. They are included within this change