-
Notifications
You must be signed in to change notification settings - Fork 769
[SYCL][CUDA] Revert https://github.com/intel/llvm/pull/6386 and remove c++17 usage. #6400
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
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
/verify with intel/llvm-test-suite#1076 |
All testing passed except validation on Windows with intel/llvm-test-suite#1076 and this Windows portion hangs currently due to machines reconfiguration. :( |
Windows issue is not relevant and same as https://github.com/intel/llvm/runs/7206588798?check_suite_focus=true |
#if __cplusplus >= 201703L | ||
if constexpr (N % 2) | ||
#else | ||
if (N % 2) | ||
#endif // __cplusplus >= 201703L | ||
{ |
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.
why do we need to be this explicit that this is a constexpr? Is this a performance critical place? Why not just remove constexpr?
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 that all math functions count as performance critical places since they will often be called many many times when used. I think that we could remove constexpr here since at least at O3 there is no difference in the final asm code whether or not constexpr is explicitly used. However I was requested in the review to use c++17 where possible.
Do you want me to remove constexpr usage 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.
I'd remove it, but I'll let you to make final call.
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.
OK I've removed it. Thanks
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.
Sorry - I've made further commits to remove some C++17 usage consistent with #6415. if constexpr
usage is necessary in joint_matrix_tensorcore.hpp.
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
Signed-off-by: JackAKirk <jack.kirk@codeplay.com>
/verify with intel/llvm-test-suite#1076 |
Reverts experimental/builtins.hpp includes added in #1072 and #1075. Requires: intel/llvm#6400
…st-suite#1076) Reverts experimental/builtins.hpp includes added in intel/llvm-test-suite#1072 and intel/llvm-test-suite#1075. Requires: intel#6400
Reverts #6386 so that experimental/builtins.hpp is included in sycl.hpp. This avoids users being required to include this header when using printf or experimental math functions.
Corresponding update here: intel/llvm-test-suite#1076.