-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[mlir][spirv] Add bfloat16 support #141458
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
Changes from 7 commits
45349e6
d9815d4
80565f0
5d2984b
3a4b936
40ae919
47ba696
49e8e86
3ed46d2
4248d7c
f2ba086
c0fee39
e01695b
ef930f4
2f8ce7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -175,10 +175,7 @@ static Type parseAndVerifyType(SPIRVDialect const &dialect, | |
|
|
||
| // Check other allowed types | ||
| if (auto t = llvm::dyn_cast<FloatType>(type)) { | ||
| if (type.isBF16()) { | ||
| parser.emitError(typeLoc, "cannot use 'bf16' to compose SPIR-V types"); | ||
| return Type(); | ||
| } | ||
| // TODO: All float types are allowed for now, but this should be fixed. | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will address this in a separate PR.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please elaborate what needs to be fixed here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current behavior does not error out on bitwidths that are invalid for SPIRV (eg. F80, F128) and non-standard formats (eg. E3M2). Do you think it's better to address this here or in a separate PR?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my opinion it's okay to address it later. In fact I think it's preferable. Currently the code doesn't do any checks anyway, other than checking for bf16, so adding a proper check would be out of scope of this PR. |
||
| } else if (auto t = llvm::dyn_cast<IntegerType>(type)) { | ||
| if (!ScalarType::isValid(t)) { | ||
| parser.emitError(typeLoc, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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 can argue that bf16 should be part of
SPIRV_Float. The problem here is that bf16 usage in SPIRV is very limited whileSPIRV_Float(i.e regular floats) is used widely in the codebase for other ops(eg. texture sampling and regular arithmetic insts). I chose to leaveSPIRV_Floatto minimize the amount of changes(and to not introduce something likeSPIRV_ArithmeticFloat). Please let me know if you think there is a cleaner solution to this.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 this, it might be better to leave them alone. what about something like this: