[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! |
|
|
||
| #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.
| #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.
| &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.
| // 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