-
-
Notifications
You must be signed in to change notification settings - Fork 299
Add predefined datatypes for bfloat16 data #5402
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
base: develop
Are you sure you want to change the base?
Add predefined datatypes for bfloat16 data #5402
Conversation
Essentially finished, pending CI results and some potential testing on a big-endian system |
doxygen/dox/DDLBNF200.dox
Outdated
H5T_IEEE_F32BE | H5T_IEEE_F32LE | | ||
H5T_IEEE_F64BE | H5T_IEEE_F64LE | | ||
H5T_NATIVE_FLOAT16 | H5T_NATIVE_FLOAT | | ||
H5T_FLT_BF16BE | H5T_FLT_BF16LE | |
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.
Open to any alternative names that seem more fitting, but this type is distinct from the IEEE standard, so I basically created a new category for alternative floating-point formats.
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.
Perhaps "FLOAT" instead of "FLT"?
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.
Seems reasonable to me
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.
Agree - I like "FLOAT" better also
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.
Actually, maybe "NONSTD", since it's a contrast to the "IEEE" label?
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.
So... H5T_NONSTD_BF16
or H5T_NONSTD_BFLOAT16
? I like "FLOAT" explicitly.
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.
The issue I see with that is that it's not unlikely that these types get adopted into some standard in the future, whether it's IEEE or not.
if (NULL == (bf16_be_dt = H5I_object(H5T_FLT_BF16BE))) | ||
HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, NULL, "not a data type"); | ||
|
||
/* Promote bfloat16 to float instead of float16, as it |
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.
bfloat16 types should be promoted to float
instead of float16, as the type is the same size as float16, but a different format. Converting between bfloat16 and float
is also very simple (by design).
if (size == 2) | ||
p_type = H5Tcopy(H5T_IEEE_F16LE); | ||
if (size == 2) { | ||
if (true == H5Tequal(tid, H5T_IEEE_F16LE) || true == H5Tequal(tid, H5T_IEEE_F16BE)) |
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.
Had to make sure the correct type between float16 and bfloat16 is picked so that the data comes out correctly.
hsize_t dims[2], adims[1]; | ||
|
||
/* | ||
* bfloat16 keeps approximately the same range as the IEEE 32-bit |
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.
Eventually better tests can be written, but for now this PR just adds support for predefined types and doesn't add support for a native type. GCC and Clang both have support for a __bf16 type at this point though, so it should be doable in the future.
I'll try to review tomorrow and get you some feedback |
It was my undesrtanding that middle part "_IEEE_" or similar represents
architecture or a standard. Should we follow the same rule here?
…On Mon, Mar 24, 2025 at 12:11 PM Quincey Koziol ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In doxygen/dox/DDLBNF200.dox
<#5402 (comment)>:
> H5T_IEEE_F32BE | H5T_IEEE_F32LE |
H5T_IEEE_F64BE | H5T_IEEE_F64LE |
- H5T_NATIVE_FLOAT16 | H5T_NATIVE_FLOAT |
+ H5T_FLT_BF16BE | H5T_FLT_BF16LE |
Agree - I like "FLOAT" better also
—
Reply to this email directly, view it on GitHub
<#5402 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADLFT3POWBKSASYVAQ4VSTD2WA4CRAVCNFSM6AAAAABZRCFMH2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDOMJRGEZTANZUHA>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Right - that was why I thought "NONSTD" might be better |
Maybe "NONSTD_BFLOAT16" ? |
I'm not sure that this is necessarily the case and I'd argue we should consider abandoning that. For example, |
That name is a bit weird to me because bfloat16 is the standard. There's no other standard for the type, it's just that the standard isn't an IEEE standard. |
Also consider I plan to add support for FP8, FP6 and FP4 following this PR, and in each case the formats essentially are the standard until they have wider adoption. https://arxiv.org/abs/2209.05433 for example. |
Since Google created it, maybe "GOOGLE_BFLOAT16" ? |
I was thinking the same... |
Frankly speaking, I would drop middle part for these types and document their implementation. I would vote against introducing GOOGLE. |
Perhaps. The reason I used BF16 on the end part was just to match our existing conventions like |
I was ok with |
For the time being, I'm going to proceed on the other datatypes using I propose that we use a convention of |
Makes sense, but I would leave old types as they are and use new standard only for the new types. |
I certainly wouldn't want to go changing old type names as it would be nothing but a source of annoyance (though we can always introduce new macros that just point to the old names if we wanted). |
adc8a52
to
8563514
Compare
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.
Pull Request Overview
This PR adds support for predefined datatypes for bfloat16 data (both little- and big-endian) and includes missing float16 predefined types for Fortran. Key changes include updates to native type retrieval in src/H5Tnative.c, new datatype definitions in Java JNI and C++ bindings, and corresponding updates in documentation and tests.
Reviewed Changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/H5Tnative.c | Updated H5T__get_native_float signature and incorporated bfloat16 support logic. |
src/H5Tmodule.h & src/H5T.c | Defined new macros and initialized new datatype IDs for bfloat16. |
java/src/jni/h5util.c | Added conditional branches for bfloat16 in datatype resolution. |
java/src/jni/h5Constants.c, HDF5Constants.java | Added JNI constant definitions for the new bfloat16 datatypes. |
hl/, fortran/, doxygen/, c++/src/ | Updated tests, parsers, documentation, and language bindings to reflect the new datatypes. |
da4a32c
to
408c6bc
Compare
408c6bc
to
10d9991
Compare
Adds predefined datatypes for little- and big-endian bfloat16 data Does not add support for any native bfloat16 types; datatype conversions are performed in software
10d9991
to
d4bbe19
Compare
Adds predefined datatypes for little- and big-endian bfloat16 data
Does not add support for any native bfloat16 types; datatype conversions are performed in software
Also adds missing float16 predefined types to fortran