-
Notifications
You must be signed in to change notification settings - Fork 768
[SYCL] add preliminary support for bfloat16 to sycl::vec #12261
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
[SYCL] add preliminary support for bfloat16 to sycl::vec #12261
Conversation
✅ With the latest revision this PR passed the C/C++ code formatter. |
Does this change enable sub_group::load and sub_group::store (https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/sub_group.hpp#L71) to work on bfloat16 type? |
@dkhaldi - I did a quick test of using sub_groups to .load() data from one buffer and modify and .store() it into another, and this works with bfloat16 in the current source base. So, if you are asking if this PR would enable that behavior, the answer is "no" - it's already present and working. AFAICT. But maybe I'm misunderstanding your inquiry? I also checked using sub_groups to .load()/.store() for |
Sorry I was not specific, Yes, I meant this latter. sub_group::load/store on |
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.
Looks much better now, but I've had too many iterations reviewing this that some things might have become invisible to my eye. @steffenlarsen , @sergey-semenov , would you mind having a look through it as well?
@intel/dpcpp-tools-reviewers could I get one of you to review? |
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.
LGTM
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.
Not sure why bfloat16
E2E test are owned by tools team, but changes LGTM
@@ -142,20 +144,61 @@ using select_apply_cl_t = std::conditional_t< | |||
template <typename T> struct vec_helper { | |||
using RetType = T; | |||
static constexpr RetType get(T value) { return value; } | |||
static constexpr RetType set(T value) { return value; } |
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.
Nit; I am not much for the name "set". What does it set? "get" isn't great for that matter, but it sounds more and more like we are mutating something, which we are not.
return std::array<DataT_, 1>{vec_data<DataT_>::get(static_cast<DataT_>(A))}; | ||
#else | ||
return std::array<DataT_, 1>{vec_data<DataT_>::get(A)}; | ||
#endif |
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.
Is there a reason we can promote this out of preview now? Was it just a mistake to have it under preview before?
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. It was a mistake and shouldn't have been there to begin with.
This does not yet add support for the math builtins for
sycl::vec<bfloat16>
. That will come later.