[SYCL][CUDA] Replace assert with CHECK #1302
Conversation
Fix LIT tests that are not compiled with assertions enabled but call assert. Signed-off-by: Bjoern Knafla <bjoern@codeplay.com>
Anyway, that's a really good thing to do. Thanks! |
| @@ -16,7 +16,7 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | |||
| // | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| #include "../../helpers.hpp" | |||
| #include <CL/sycl.hpp> | |||
| #include <cassert> | |||
There was a problem hiding this comment.
| #include <cassert> |
Do we still need this include?
There was a problem hiding this comment.
It shouldn’t be necessary - I will fix this on Monday.
| @@ -13,9 +13,9 @@ | |||
| #include <detail/kernel_impl.hpp> | |||
| #include <detail/scheduler/scheduler.hpp> | |||
|
|
|||
| #include "../helpers.hpp" | |||
There was a problem hiding this comment.
What about replacing assert in this file?
There was a problem hiding this comment.
This is probably (cannot check at the moment) an artifact of breaking a far too large for its own good PR into smaller PRs. I will check if that is the case and repair it.
| @@ -42,9 +43,10 @@ int getExpectedQueueNumber(cl_device_id device_id, int default_value) { | |||
| sizeof(reportedProps), | |||
| &reportedProps, | |||
| NULL); | |||
| assert(CL_SUCCESS == iRet && "Failed to obtain queue info from ocl device"); | |||
| CHECK(CL_SUCCESS == iRet && "Failed to obtain queue info from ocl device"); | |||
There was a problem hiding this comment.
Why only part of the asserts are replaced in this file?
There was a problem hiding this comment.
Thank you for catching this. Breaking a larger PR down into smaller ones I might have lost some changes (or I was sloppy in the first place). I will look into it.
| @@ -11,6 +11,7 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | |||
| // | |||
| //===----------------------------------------------------------------------===// | |||
| #include "../helpers.hpp" | |||
There was a problem hiding this comment.
Consider adding directory with "helpers.hpp" to the list of include directories(aka -I).
There was a problem hiding this comment.
@bjoernknafla Will you do it or we merging as is?
There was a problem hiding this comment.
I am currently experiencing many LIT failures on the sycl branch and am investigating before I can rebase.
As @bader pointed out in another PR, also there is no need to replace assert as it isn't turned off when not explicitly defining the NDEBUG preprocessor symbol. I was confused as CMake has extra behavior but these LIT tests do not go through CMake. I am considering rejecting and closing this PR.
|
As @bader pointed out here: #1304 (comment) - it is compeltely fine to use |
…iews.llvm.org/D114631 (intel#1302) Co-authored-by: yubingex007-a11y <bing1.yu@intel.com> Original commit: KhronosGroup/SPIRV-LLVM-Translator@0fdea74
Fix LIT tests that are not compiled with assertions enabled but call
assert.
Only done for LIT tests that requires fixes during work on CUDA.
Signed-off-by: Bjoern Knafla bjoern@codeplay.com