-
Notifications
You must be signed in to change notification settings - Fork 380
Implement builtin type bfloat16_t #8757
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: master
Are you sure you want to change the base?
Conversation
|
This PR modifies IR instruction definition files. Please review if you need to update the following constants in
These version numbers help ensure compatibility between different versions of compiled modules. |
0f2abeb to
60e1b36
Compare
jkwak-work
left a comment
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 have a few comments but it looks very good to me.
Although it looks very reasonable to me, I am not sure if we like to use "bf" as its suffix yet.
|
|
||
| INT_MASK = SINT_MASK | UINT_MASK, | ||
| ARITHMETIC_MASK = INT_MASK | FLOAT_MASK, | ||
| ARITHMETIC_MASK = INT_MASK | FLOAT_MASK | BFLOAT_MASK, |
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.
Correct me if I have misunderstanding, but as far as I understand, bfloat16_t is only for Cooperative-vector/matrix.
I don't think we should treat it as a regular arithmetic type.
I also see the following statement from GL_EXT_bfloat16:
Arithmetic operators are not supported on bfloat16_t, or vectors of that type, or cooperative matrices of that type.
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 also where I'm confused. Both CoopMat and CoopVec require the component type to be arithmetic type. My understanding is that we must implement some functions to make bfloat16 as arithmetic type. However, SPIR-V / glslang does not support these functions...
Therefore, I wrote in the description:
Since spir-v doesn't allows arithmetic ops for bfloat16, it will emit invalid spir-v code, but slang has no restrictions.
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 see why you added it.
Currently the CoopVec uses __BuiltinArithmeticType:
struct CoopVec<T : __BuiltinArithmeticType, let N : int> : IArray<T>, IArithmetic
I think there are two options here: a) add a new type just for CoopVec operation or b) use __BuiltinType and add a few static_assert to cause errors when unsupported types are used for CoopVec such as bool and double.
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.
Hmm.. I tried a bit and it turned to be not trivial to use either way.
Probably the current implementation is good enough.
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.
@csyonghe suggested that we should have a new type just for CoopVec, ICoopVecElement.
Because bfloat16 isn't IArithmetic type.
Similarly texture uses ITexelElement.
You can check it out of how a new type like that can be added.
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.
@csyonghe suggested that we should have a new type just for CoopVec, ICoopVecElement.
Thanks, it looks better. I think we need a IMLElement or other interface. CoopMat also requires it. I will implement IMLElement first, please let me know if we have a better interface name.
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.
We had a bit more discussion about the naming,
IMLElement is too broad and we prefer it to be ICoopVecElement.
If we want to have a general ML operators in the future, we will regret giving a broad name to this specific narrow scope.
But we are still open to suggestions for a better naming at the moment.
Also at the same topic, this PR will not be able to have default implementation after switching to whatever the new interface name becomes.
Because the idea is that the new interface is not IArithmetic and we cannot simply provide a default implementation anymore.
As an example, a simple addition operation like CoopVec<float,5> + CoopVec<float,5> cannot have a default implementation.
That means this PR will be a backward compatibility breaking change.
I will add a "breaking change" label, when the change is reviewed again.
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 agree with most of the points.
IMLElementis too broad and we prefer it to beICoopVecElement.
My understanding is CoopMat requires this, and CoopVec is only one of these usages. So I'd like to call it MLElement, OTOW does not bundle CoopVec. For example, CoopMat<bfloat16_t, Subgroup, 16, 16, UseA> is valid type in the real world.
As an example, a simple addition operation like
CoopVec<float,5> + CoopVec<float,5>cannot have a default implementation.
Yes.
Perhaps we can try to allow operator+ if component is arithmetic types, otherwise not. (Maybe in the future)
jkwak-work
left a comment
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 noticed that there isn't any tests with CoopVec, which is the main reason of having bfloat16_t.
Can you add some tests with CoopVec; and possibly with CoopMat as well?
GinShio
left a comment
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.
Although it looks very reasonable to me, I am not sure if we like to use "bf" as its suffix yet.
I'd like to add bf16, that's same as C++23 or glsl. But seems to hard add number suffix.
Can you add some tests with CoopVec; and possibly with CoopMat as well?
This seems to be a bit harder for CoopVec using bfloat16 matrix/bias. Added a simple coopvec test.
|
|
||
| INT_MASK = SINT_MASK | UINT_MASK, | ||
| ARITHMETIC_MASK = INT_MASK | FLOAT_MASK, | ||
| ARITHMETIC_MASK = INT_MASK | FLOAT_MASK | BFLOAT_MASK, |
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 also where I'm confused. Both CoopMat and CoopVec require the component type to be arithmetic type. My understanding is that we must implement some functions to make bfloat16 as arithmetic type. However, SPIR-V / glslang does not support these functions...
Therefore, I wrote in the description:
Since spir-v doesn't allows arithmetic ops for bfloat16, it will emit invalid spir-v code, but slang has no restrictions.
|
@GinShio Does this need review? I see that there is a branch conflict with |
|
I am changing the status to "draft" because the current PR has a conflict to the current files at ToT. |
This PR implements the
bfloat16_ttype for slang and emit it to SPIR-V. The literal suffix isbf. According to spir-v specification, it can be used in cooperative matrix and instruction dot.Since spir-v doesn't allows arithmetic ops for bfloat16, it will emit invalid spir-v code, but slang has no restrictions. Also not support type reflection.