Adding support for binary8 #4
Conversation
Specifically, e4m3 in the OCP standard does not represent infinities to expand the maximum exponent to 8.
This allows to make fp8-e4m3 fully OCP compliant, where it is specified with emax = 8 and emin = -6.
src/cpfloat_definitions.h
Outdated
| * @brief Minimum exponent of target format. | ||
| * | ||
| * @details The minimum values allowed are -126 and -1022 if the storage format | ||
| * is `float` or `double`, respectively. Smaller values are increase to the |
There was a problem hiding this comment.
increased? Perhaps I would change this sentence to read If a smaller value is chosen, this is changed to the minimum allowed value without warning. And probably, the emax docstring should be changed accordingly – I find it's not the clearest description of what's happening.
src/cpfloat_template.h
Outdated
| @@ -106,6 +106,7 @@ optstruct *init_optstruct() { | |||
| fpopts->bitseed = NULL; | |||
| fpopts->randseedf = NULL; | |||
| fpopts->randseed = NULL; | |||
| fpopts->emin = -99999; | |||
There was a problem hiding this comment.
Not sure I like this magic constant – would 0 do as well here? emin should be strictly negative anyway.
| @@ -279,6 +280,10 @@ static inline int VALIDATE_INPUT(const optstruct *fpopts) { | |||
| if (fpopts->flip != CPFLOAT_NO_SOFTERR && (fpopts->p > 1 || fpopts->p < 0)) | |||
| return 5; | |||
|
|
|||
| /* Return -6 if emin is invalid (either nonnegative or too small). */ | |||
| if (fpopts->emin < DEFEMIN || fpopts->emin >= 0) | |||
There was a problem hiding this comment.
No changes needed here, as far as I can tell.
src/cpfloat_template.h
Outdated
| int emin = 1-emax; | ||
| int emin = fpopts->emin; | ||
| /* If emin is not set by user, set it to the default 1-emax. */ | ||
| if (emin == -99999) |
There was a problem hiding this comment.
This -9999 would become a 0, I think.
src/cpfloat_template.h
Outdated
| @@ -304,7 +309,14 @@ static inline FPPARAMS COMPUTE_GLOBAL_PARAMS(const optstruct *fpopts, | |||
| } | |||
|
|
|||
| /* Derived floating point parameters. */ | |||
| int emin = 1-emax; | |||
| int emin = fpopts->emin; | |||
| /* If emin is not set by user, set it to the default 1-emax. */ | |||
There was a problem hiding this comment.
Should this be the user?
| @@ -1199,7 +1211,7 @@ GENERATE_UNIVARIATE_MATH_H(lgamma, lgamma) | |||
| if (temp == FP_ILOGB0 || temp == FP_ILOGBNAN || temp == INT_MAX) { \ | |||
| params.precision = DEFPREC-1; \ | |||
| params.emax = DEFEMAX; \ | |||
| params.emax = DEFEMIN; \ | |||
| params.emin = DEFEMIN; \ | |||
There was a problem hiding this comment.
Thanks for this – can you change the message of the last commit to
Fixed the emin bug (closes #2)
I think I've found the issue and fixed it locally. I can push this to |
Please push it to main. I will rebase onto it once done here. |
|
Looks good, thanks! I'll rebase and merge – the code should pass the revised tests, and if not, we can fix it later. |
|
Mhh – it didn't quite work. It seems that rebase and merge doesn't actually rebase on |
I'm creating a pull request so that we can comment on changes before rebasing.