Intoducing bfloat16 type #21522
Intoducing bfloat16 type #21522
Conversation
pytorchbot
added
the
module: internals
Intoducing bfloat16 type gh-metadata: pytorch pytorch 21522 gh/izdeby/8/head
Intoducing bfloat16 type gh-metadata: pytorch pytorch 21522 gh/izdeby/8/head
Intoducing bfloat16 type gh-metadata: pytorch pytorch 21522 gh/izdeby/8/head
gchanan
left a comment
gchanan
left a comment
There was a problem hiding this comment.
this looks pretty good!
It would be great if:
- you could do use memcpy, as suggested by @ezyang.
- you look into big endianness; we don't test it (which is fine for now, you can even assert in your bfloat16_test for now), but it should be only a few line change and you can just test it once.
Intoducing bfloat16 type gh-metadata: pytorch pytorch 21522 gh/izdeby/8/head
c10/util/BFloat16.h
Outdated
|
|
||
| namespace detail { | ||
| inline bool isSmallEndian() { | ||
| int num = 1; |
There was a problem hiding this comment.
you should test this statically.
There was a problem hiding this comment.
You link https://stackoverflow.com/questions/1583791/constexpr-and-endianness presumably because you wanted to implement this using constexpr. But I don't think we actually are requiring you to implement it with constexpr. All we are asking is that there isn't actually a conditional check in the generated machine code. I don't know if the current implementation you have here, it is managed to be compiled away; if it is, that would be sufficient for me (it's a good thing to put in a comment.)
Intoducing bfloat16 type gh-metadata: pytorch pytorch 21522 gh/izdeby/8/head
|
Would this PR consider operator overloading like |
|
@ezyang, @gchanan, @colesbury can you guys, please, take another look at this PR? Important to note: |
|
LGTM modulo comments. |
Intoducing bfloat16 type gh-metadata: pytorch pytorch 21522 gh/izdeby/8/head
Intoducing bfloat16 type gh-metadata: pytorch pytorch 21522 gh/izdeby/8/head
|
It might make sense to do rne conversion rather than rtz (via @benbarsdell) https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/lib/bfloat16/bfloat16.h#L178-L184 |
c10/test/util/bfloat16_test.cpp
Outdated
| c10::BFloat16 a = c10::BFloat16(in); | ||
| float out = c10::detail::f32_from_bits(a.x); | ||
|
|
||
| EXPECT_FLOAT_EQ(1.1663108e-38, out); |
There was a problem hiding this comment.
where does this number come from?
There was a problem hiding this comment.
I'd comment on what you are trying to test here.
There was a problem hiding this comment.
Killed this test as it not too representative now when i think about it.
Intoducing bfloat16 type gh-metadata: pytorch pytorch 21522 gh/izdeby/8/head
1 similar comment
Stack from ghstack:
Introducing BFloat16 tensor type
BFoat16 is a float32 truncated to its first 16 bits. So it has the same 8 bits for exponent, and only 7 bits for mantissa. It is therefore easy to convert from and to float32, and because it has basically the same range as float32, it minimizes the risks of having NaNs or exploding/vanishing gradients when switching from float32. In addition, you can use the bfloat16 format to accurately represent all integers [-256, 256], which means you can encode an int8 in bfloat16 without loss of accuracy.
In this PR:
Differential Revision: D15819369