-
Notifications
You must be signed in to change notification settings - Fork 752
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] Rework 'half' implementation in order to remove bunch of workarounds #1089
[SYCL] Rework 'half' implementation in order to remove bunch of workarounds #1089
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.
Overall LGTM
b07dd2c
to
970468a
Compare
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 aside from some nitpicks
The only file left is `sycl/test/regression/fp16-with-unnamed-lambda.cpp` Signed-off-by: Alexey Sachkov <[email protected]>
Changes to tests are preserved Signed-off-by: Alexey Sachkov <[email protected]>
Because of the fact, that `half` type is not a standard C++ type and it is not supported everywhere, its implementation differs between host and device: C++ class with overloaded arithmetic operators is used on host and `_Float16` is used on device side. Previously, the switch between two version was implemented as preprocessor macro and having two different types caused some problems with integration header and unnamed lambda feature, see intel#185 and intel#960. This patch redesigned `half` implementation in a way, that single wrapper data type is used as `half` representation on both host and device sides; differentiation between actual host and device implementations is done under the hood of this wrapper. Signed-off-by: Alexey Sachkov <[email protected]>
970468a
to
5d8ac20
Compare
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.
+1 for compiler changes.
Great work, @AlexeySachkov! |
Error was reproducible in two cases: - using something like `numeric_limits<half>::min()` in within another `constexpr` - not treating SYCL headers as system ones with `-Winvalid-constexpr` treated as error Signed-off-by: Alexey Sachkov <[email protected]>
Error was reproducible in two cases: - using something like `numeric_limits<half>::min()` in within another `constexpr` - not treating SYCL headers as system ones with `-Winvalid-constexpr` treated as error Signed-off-by: Alexey Sachkov <[email protected]>
* Implement SPV_INTEL_debug_module extension Spec intel#3976 Original commit: KhronosGroup/SPIRV-LLVM-Translator@2b3a2f3
Because of the fact, that
half
type is not a standard C++ type and it isnot supported everywhere, its implementation differs between host and
device: C++ class with overloaded arithmetic operators is used on host
and
_Float16
is used on device side.Previously, the switch between two version was implemented as
preprocessor macro and having two different types caused some problems
with integration header and unnamed lambda feature, see #185 and
#960 (these patches are partially reverted in this PR).
This PR redesigned
half
implementation in a way, that singlewrapper data type is used as
half
representation on both host anddevice sides; differentiation between actual host and device
implementations is done under the hood of this wrapper.