-
Notifications
You must be signed in to change notification settings - Fork 794
[SYCL] Add static cast for half builtins relational #6133
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
Conversation
|
@yanfeng3721, ping |
| namespace { | ||
|
|
||
| template <typename T> inline T __vFOrdEqual(T x, T y) { return -(x == y); } | ||
| template <typename T> inline T __vFOrdEqual(T x, T y) { |
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.
should these return a bool or T?
Where in the spec, this is stated?
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.
__vFOrdEqual is not stated in the spec. I am not sure as to if this should return a bool or T.
The goal of this change to resolve an issue with building DPCPP on windows and not to change any functionality.
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.
All these are part of 4.17.9. Relational functions. Some of them should return bool. However, you are right that the PR does not change any functionality. If there is a a problem, it was already there in the original code.
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 think functions modified here are all ok, because they're vector and vector versions don't return bool at all (even all/any).
As for the scalar version (not touched in this PR), this is slightly more complicated as SYCL1.2.1 and SYCL2020 differ here. Those, however, are all internal details and not the user-visible routines so technically we can have them defined whatever way we want to be able to implement user-visible wrappers in a way dependent on the presence of SYCL2020_CONFORMANT_APIS macro.
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.
BTW, my guess why it returns -1 is because it is often good for vectorizer to have element filled with ones or with zeroes for using in vector mask operations like VANDPS/VORPS,
but I also was puzzled seeing return of -1.0 for float comparison.
It would be logically more correct to returning int32_t{-1} for float32 rather than -1.0
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.
User-visible methods must do exactly that according to the spec. I guess/think/hope that only those internal functions behave differently for some reason.
dkhaldi
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.
While there seem to be ambiguity on the return type for these SYCL relational functions, this specific change to solve the build bugs on Windows LGTM.
I would suggest opening a new PR to check and correct the return type if something is wrong.
|
This PR is supposed to fix the build errors on Windows, but CI still fails: http://llvm-ci-test2.intel.com:8080/blue/organizations/jenkins/SYCL_CI%2Fintel%2FWin%2FTest_Suite/detail/Test_Suite/1869/pipeline `[2022-05-10T16:33:14.773Z] W:\jenkins-dir\workspace\SYCL_CI\intel\Win\Test_Suite\llvm.src\sycl\source\detail\builtins_relational.cpp(230): error C2220: the following warning is treated as an error [2022-05-10T16:33:14.773Z] W:\jenkins-dir\workspace\SYCL_CI\intel\Win\Test_Suite\llvm.src\sycl\source\detail\builtins_relational.cpp(230): warning C4804: '-': unsafe use of type 'bool' in operation [2022-05-10T16:33:14.773Z] W:\jenkins-dir\workspace\SYCL_CI\intel\Win\Test_Suite\llvm.src\sycl\source\detail\builtins_relational.cpp(24): warning C4804: '-': unsafe use of type 'bool' in operation [2022-05-10T16:33:14.773Z] W:\jenkins-dir\workspace\SYCL_CI\intel\Win\Test_Suite\llvm.src\sycl\source\detail\builtins_relational.cpp(155): note: see reference to function template instantiation 'T cl::__host_std::`anonymous-namespace'::__vFOrdEqualcl::sycl::cl_half(T,T)' being compiled [2022-05-10T16:33:14.773Z] with [2022-05-10T16:33:14.773Z] [ [2022-05-10T16:33:14.773Z] T=cl::sycl::cl_half [2022-05-10T16:33:14.773Z] ] [2022-05-10T16:33:14.773Z] W:\jenkins-dir\workspace\SYCL_CI\intel\Win\Test_Suite\llvm.src\sycl\source\detail\builtins_relational.cpp(28): warning C4804: '-': unsafe use of type 'bool' in operation` |
|
I've checked locally that it helps before merging. I think it fails for reference compiler, that's the only possible explanation I have. |
Whether it fails for reference compiler or not must be clear from the logs. |
This PR adds a static cast to builtins_relational functions which apply a negative operation to a boolean type.
This is only applied to functions which are specialised for cl_half. The cast prevents windows error message
C4804.This resolves a windows build issue introduced by #6061