From ab11500fde2fdf03f195c2866abeb3d3871dbcae Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Thu, 11 Sep 2025 10:01:29 -0700 Subject: [PATCH 1/2] [SYCL] Follow-up for https://github.com/intel/llvm/pull/19990 Downstream CI uncovered another scenario where `detail::CompileTimeKernelInfo.Name` might be empty (other than non-SYCL compilation in some of the [unit]tests). That happens when compiling the same TU using multiple offload models (e.g., both SYCL and OMP) and the device compilation for non-SYCL model doesn't use SYCL integration header to provide kernel information. Also, my previous comments about `__usmfill` turned out to be incorrect. We do NOT unconditionally instantiate any of those in the `sycl/handler.hpp`, it was individual unittests that did that. --- sycl/include/sycl/handler.hpp | 29 +++++++++++++++-------------- sycl/test/lit.cfg.py | 4 +--- sycl/unittests/CMakeLists.txt | 2 -- 3 files changed, 16 insertions(+), 19 deletions(-) diff --git a/sycl/include/sycl/handler.hpp b/sycl/include/sycl/handler.hpp index 44b46d4960910..9df82f8b1001c 100644 --- a/sycl/include/sycl/handler.hpp +++ b/sycl/include/sycl/handler.hpp @@ -836,26 +836,28 @@ class __SYCL_EXPORT handler { Dims>()); #endif - // SYCL unittests are built without sycl compiler, so "host" information - // about kernels isn't provided (e.g., via integration headers or compiler - // builtins). - // - // However, some copy/fill USM operation are implemented via SYCL kernels - // and are instantiated resulting in all the `static_assert` checks being - // exercised. Without kernel information that would fail, so we explicitly - // disable such checks when this macro is defined. Note that the unittests - // don't actually execute those operation, that's why disabling - // unconditional `static_asserts`s is enough for now. -#ifndef __SYCL_UNITTESTS_BYPASS_KERNEL_NAME_CHECK constexpr auto Info = detail::CompileTimeKernelInfo; - static_assert(Info.Name != std::string_view{}, "Kernel must have a name!"); + // Ideally, the following should be a `static_assert` but cannot do that as + // it would fail in at least two scenarios: + // + // * Our own unittests that are compiled with an arbitrary host compiler + // without SYCL knowledge (i.e., no integration headers + // generated/supplied). Those that don't have to supply any stub kernel + // information test exceptions thrown before kernel enqueue, so having a + // run-time assert here doesn't affect them. + // + // * Mixed SYCL/OpenMP compilation, device code generation for OpenMP in + // * particular. For that scenario this code is compiled but never + // * executed. + assert(Info.Name != std::string_view{} && "Kernel must have a name!"); // Some host compilers may have different captures from Clang. Currently // there is no stable way of handling this when extracting the captures, // so a static assert is made to fail for incompatible kernel lambdas. static_assert( - sizeof(KernelType) == Info.KernelSize, + Info.Name == std::string_view{} || + sizeof(KernelType) == Info.KernelSize, "Unexpected kernel lambda size. This can be caused by an " "external host compiler producing a lambda with an " "unexpected layout. This is a limitation of the compiler." @@ -866,7 +868,6 @@ class __SYCL_EXPORT handler { "In case of MSVC, passing " "-fsycl-host-compiler-options='/std:c++latest' " "might also help."); -#endif setDeviceKernelInfo((void *)MHostKernel->getPtr()); diff --git a/sycl/test/lit.cfg.py b/sycl/test/lit.cfg.py index 94408de4ec328..4ebe0b06ff040 100644 --- a/sycl/test/lit.cfg.py +++ b/sycl/test/lit.cfg.py @@ -126,9 +126,7 @@ llvm_symbolizer = os.path.join(config.llvm_build_bin_dir, "llvm-symbolizer") llvm_config.with_environment("LLVM_SYMBOLIZER_PATH", llvm_symbolizer) -sycl_host_only_options = ( - "-std=c++17 -Xclang -fsycl-is-host -D__SYCL_UNITTESTS_BYPASS_KERNEL_NAME_CHECK=1" -) +sycl_host_only_options = "-std=c++17 -Xclang -fsycl-is-host" for include_dir in [ config.sycl_include, config.level_zero_include_dir, diff --git a/sycl/unittests/CMakeLists.txt b/sycl/unittests/CMakeLists.txt index 6002f48d214a7..62af3d0074cac 100644 --- a/sycl/unittests/CMakeLists.txt +++ b/sycl/unittests/CMakeLists.txt @@ -9,8 +9,6 @@ endforeach() add_compile_definitions(SYCL2020_DISABLE_DEPRECATION_WARNINGS SYCL_DISABLE_FSYCL_SYCLHPP_WARNING) -add_compile_definitions(__SYCL_UNITTESTS_BYPASS_KERNEL_NAME_CHECK) - # suppress warnings which came from Google Test sources if (CXX_SUPPORTS_SUGGEST_OVERRIDE_FLAG) add_compile_options("-Wno-suggest-override") From faad5e751b1f6e73f107a19649587a04dfd0ecb6 Mon Sep 17 00:00:00 2001 From: Andrei Elovikov Date: Fri, 12 Sep 2025 07:12:28 -0700 Subject: [PATCH 2/2] Update sycl/include/sycl/handler.hpp Co-authored-by: Sergey Semenov --- sycl/include/sycl/handler.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sycl/include/sycl/handler.hpp b/sycl/include/sycl/handler.hpp index 9df82f8b1001c..004b122b80a44 100644 --- a/sycl/include/sycl/handler.hpp +++ b/sycl/include/sycl/handler.hpp @@ -848,8 +848,8 @@ class __SYCL_EXPORT handler { // run-time assert here doesn't affect them. // // * Mixed SYCL/OpenMP compilation, device code generation for OpenMP in - // * particular. For that scenario this code is compiled but never - // * executed. + // particular. For that scenario this code is compiled but never + // executed. assert(Info.Name != std::string_view{} && "Kernel must have a name!"); // Some host compilers may have different captures from Clang. Currently