[L0][UR] Only Override max allocation limits given env

[nrspruit]

pre merge CI PR for oneapi-src/unified-runtime#1245

[kbenzie]

Okay, I've managed to reproduce this Windows compile error locally. I'll work on a fix to UR and prioritise it.

[kbenzie]

An include changed which also includes Windows.h, its defining min. Fix will be trivial.

[kbenzie]

oneapi-src/unified-runtime#1258 should do the trick.

[kbenzie]

oneapi-src/unified-runtime#1258 should do the trick.

Actually this isn't required, we already define NOMINMAX in the UR CMake but its not replicated in the PI plugin CMake for L0. If you apply this patch @nrspruit we should be able to merge this sooner:

diff --git a/sycl/plugins/level_zero/CMakeLists.txt b/sycl/plugins/level_zero/CMakeLists.txt
index 5c24988813c1..19a75b4736bd 100644
--- a/sycl/plugins/level_zero/CMakeLists.txt
+++ b/sycl/plugins/level_zero/CMakeLists.txt
@@ -102,6 +102,10 @@ add_sycl_plugin(level_zero
 if (WIN32)
   # 0x800: Search for the DLL only in the System32 folder
   target_link_options(pi_level_zero PUBLIC /DEPENDENTLOADFLAG:0x800)
+  target_compile_definitions(pi_level_zero PRIVATE
+    WIN32_LEAN_AND_MEAN
+    NOMINMAX
+  )
 endif()

 add_dependencies(pi_level_zero ze-api)

[nrspruit]

oneapi-src/unified-runtime#1258 should do the trick.

Actually this isn't required, we already define NOMINMAX in the UR CMake but its not replicated in the PI plugin CMake for L0. If you apply this patch @nrspruit we should be able to merge this sooner:

diff --git a/sycl/plugins/level_zero/CMakeLists.txt b/sycl/plugins/level_zero/CMakeLists.txt
index 5c24988813c1..19a75b4736bd 100644
--- a/sycl/plugins/level_zero/CMakeLists.txt
+++ b/sycl/plugins/level_zero/CMakeLists.txt
@@ -102,6 +102,10 @@ add_sycl_plugin(level_zero
 if (WIN32)
   # 0x800: Search for the DLL only in the System32 folder
   target_link_options(pi_level_zero PUBLIC /DEPENDENTLOADFLAG:0x800)
+  target_compile_definitions(pi_level_zero PRIVATE
+    WIN32_LEAN_AND_MEAN
+    NOMINMAX
+  )
 endif()

 add_dependencies(pi_level_zero ze-api)

Hello @kbenzie , it looks like this flag is also needed for the opencl plugin. Should this be a larger patch for all plugins?

[nrspruit]

oneapi-src/unified-runtime#1258 should do the trick.

Actually this isn't required, we already define NOMINMAX in the UR CMake but its not replicated in the PI plugin CMake for L0. If you apply this patch @nrspruit we should be able to merge this sooner:

diff --git a/sycl/plugins/level_zero/CMakeLists.txt b/sycl/plugins/level_zero/CMakeLists.txt
index 5c24988813c1..19a75b4736bd 100644
--- a/sycl/plugins/level_zero/CMakeLists.txt
+++ b/sycl/plugins/level_zero/CMakeLists.txt
@@ -102,6 +102,10 @@ add_sycl_plugin(level_zero
 if (WIN32)
   # 0x800: Search for the DLL only in the System32 folder
   target_link_options(pi_level_zero PUBLIC /DEPENDENTLOADFLAG:0x800)
+  target_compile_definitions(pi_level_zero PRIVATE
+    WIN32_LEAN_AND_MEAN
+    NOMINMAX
+  )
 endif()

 add_dependencies(pi_level_zero ze-api)

Hello @kbenzie , it looks like this flag is also needed for the opencl plugin. Should this be a larger patch for all plugins?

after updating both plugins opencl and level zero there is a new error because this is set elsewhere:
D:\github_work\llvm\llvm\src\xpti\include\xpti/xpti_trace_framework.hpp(25): error C2220: the following warning is treated as an error
D:\github_work\llvm\llvm\src\xpti\include\xpti/xpti_trace_framework.hpp(25): warning C4005: 'NOMINMAX': macro redefinition
D:\github_work\llvm\llvm\src\xpti\include\xpti/xpti_trace_framework.hpp(25): note: 'NOMINMAX' previously declared on the command line

[kbenzie]

after updating both plugins opencl and level zero there is a new error because this is set elsewhere:
D:\github_work\llvm\llvm\src\xpti\include\xpti/xpti_trace_framework.hpp(25): error C2220: the following warning is treated as an error
D:\github_work\llvm\llvm\src\xpti\include\xpti/xpti_trace_framework.hpp(25): warning C4005: 'NOMINMAX': macro redefinition
D:\github_work\llvm\llvm\src\xpti\include\xpti/xpti_trace_framework.hpp(25): note: 'NOMINMAX' previously declared on the command line

Hmm, perhaps we need to move the windows.h include out of ur_util.hpp to avoid it being transitively included all over the place. I'll try that out.

[kbenzie]

Since some functions moved out of the ur_util.hpp header the pi_unified_runtime target now needs to link against the UnifiedRuntimeCommon library. This should fix this link error.

diff --git a/sycl/plugins/unified_runtime/CMakeLists.txt b/sycl/plugins/unified_runtime/CMakeLists.txt
index 8d7f2f32b415..b2bb94714c0b 100644
--- a/sycl/plugins/unified_runtime/CMakeLists.txt
+++ b/sycl/plugins/unified_runtime/CMakeLists.txt
@@ -150,6 +150,7 @@ set(UNIFIED_RUNTIME_PLUGIN_ARGS
     Threads::Threads
     UnifiedRuntimeLoader
     UnifiedRuntime-Headers
+    UnifiedRuntimeCommon
   INCLUDE_DIRS
     "${UNIFIED_RUNTIME_SRC_INCLUDE_DIR}"
     "${UNIFIED_RUNTIME_COMMON_INCLUDE_DIR}"

[kbenzie]

My local build did not have SYCL_ENABLE_XPTI_TRACING=ON so they were not failing in this way. I've enabled that now and reproduced the error, I'm not really sure why its there yet but looking into it now.

[kbenzie]

I've reverted the CMake changes since they were no logner required after moving the Windows.h include, this should also fix the macro redefinition error.

[aelovikov-intel]

I see

SYCL :: DeviceLib/string_test.cpp

failing in post-commit after this PR on Intel Arc A-series GPU. Please fix ASAP, it's been ignored for two days...

@nrspruit @againull

[nrspruit]

I see

SYCL :: DeviceLib/string_test.cpp

failing in post-commit after this PR on Intel Arc A-series GPU. Please fix ASAP, it's been ignored for two days...

@nrspruit @againull

Do you have a link to the error or an error log?

[aelovikov-intel]

Do you have a link to the error or an error log?

https://github.com/intel/llvm/actions/runs/7588321461/job/20670983110