-
Notifications
You must be signed in to change notification settings - Fork 802
[SYCL][NFC] More sub_group_mask.hpp
cleanup
#18975
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][NFC] More sub_group_mask.hpp
cleanup
#18975
Conversation
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.
…lify-sub-group-mask-header
@intel/llvm-gatekeepers please consider merging |
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.
Just a nit.
@@ -378,19 +378,11 @@ group_ballot([[maybe_unused]] Group g, [[maybe_unused]] bool predicate) { | |||
#ifdef __SYCL_DEVICE_ONLY__ | |||
return sycl::detail::commonGroupBallotImpl(g, predicate); | |||
#else | |||
throw exception{errc::feature_not_supported, | |||
"Sub-group mask is not supported on host device"}; | |||
// Groups are not user-constructible, this call should not be reachable from |
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.
Would it be worth adding llvm_unreachable
here?
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.
Two concerns here:
llvm_unreachable
comes from LLVM headers that we don't ship (and probably don't want to)- Any "early exits" in form of exception/unreachable may affect host compilation - I'm afraid of unintended side effects like we saw in [SYCL] Fix SYCL_EXTERNAL device code when linking with a static lib #14256
No description provided.