[SYCL] Warning if use sycl without dynamic C++ RT on Windows #2501

v-klochkov · 2020-09-21T05:35:09Z

With -fsycl switch the dynamic runtime is used.
The patches (#2478, #2480, #2497 implemented that with -fsycl.

Applications using sycl headers and linked with sycl[d].dll must be linked
with dynamic C++ runtime on Windows even if compiled without -fsycl.
This patch adds a compile time warning emitted when wrong C++ runtime is used.

Signed-off-by: Vyacheslav N Klochkov [email protected]

romanovvlad · 2020-09-21T07:20:23Z

/summary:run

vladimirlaz · 2020-09-21T12:04:39Z

sycl/test/basic_tests/buffer/buffer.cpp

@@ -1,5 +1,3 @@
-// RUN: %clangxx %s -o %t1.out -lsycl -I %sycl_include


Did we prohibit use of the compile to build C++ code? Otherwise we do not need to remove this code.

This command compiles a test with static C++ RT on Windows, which causes an assert in stl.hpp
(the assert is added in this PR).

Vladimir, Do you have good ideas on how to allow such compilation on Linux (where default C++ RT is dynamic) and not allow on Windows without -fsycl (where default C++ RT is static)?

We have OS specific placehoders for OpenCL CPU: https://github.com/intel/llvm/blob/sycl/sycl/test/lit.cfg.py#L144
You can have some general LINUX placeholder to run the compilation only on Linux.

But it is not perfect solution so I will not insist on implementing it.

I did not find any good way to detect if the compilation runs with -fsyntax-only or without it.

vladimirlaz · 2020-09-21T12:07:28Z

sycl/test/basic_tests/implicit_conversion_error.cpp

@@ -1,4 +1,4 @@
-// RUN: %clangxx -fsyntax-only -Xclang -verify  %s -I %sycl_include -Xclang -verify-ignore-unexpected=note,warning


Do we really need to change this test as it is compile-time check?

The new static_assert at stl.hpp is emitted on Windows when a) user does not use -fsycl and b) when such compilation would potentially lead to linking with static RT.
I will check if there is any difference in compilation macros with/with -fsyntax-only and if that difference usable in stl.hpp to not give an error with -fsyntax-only.

sycl/include/CL/sycl/stl.hpp

bader · 2021-02-12T10:20:10Z

@v-klochkov, could you resolve merge conflicts and address comments from @romanovvlad, please?

…t -fsycl With -fsycl switch the dynamic runtime is used. The patches (intel#2478, intel#2480, intel#2497 implemented that with -fsycl. This patch adds check that dynamic runtime is used when -fsycl is not used. That is done by a static_assert in sycl/stl.hpp. Also, lots of LIT tests had to be changed to comply with the new requirement (apps must use dynamic C++ RT with use SYCL). Signed-off-by: Vyacheslav N Klochkov <[email protected]>

sycl/include/CL/sycl/stl.hpp

alexbatashev · 2021-03-29T07:47:46Z

sycl/include/CL/sycl/stl.hpp

+// which is not guaranteed to work and considered not safe in general.
+// Only using same dynamic C++ runtime library for sycl[d].dll and for
+// the application using sycl[d].dll is guaranteed to work properly.
+inline constexpr bool isMSVCDynamicCXXRuntime() {


constexpr implies inline

Suggested change

inline constexpr bool isMSVCDynamicCXXRuntime() {

constexpr bool isMSVCDynamicCXXRuntime() {

sycl/test/basic_tests/context.cpp

Signed-off-by: Vyacheslav N Klochkov <[email protected]>

…md_stl_static_assert

kbobrovs

sycl/test/esimd/vadd.cpp LGTM.

v-klochkov · 2021-03-30T05:52:32Z

3 or 4 failed tests from the Jenkins/Precommit use separate compilation, e.g.:
https://github.com/intel/llvm-test-suite/blob/intel/SYCL/Config/kernel_from_file.cpp
https://github.com/intel/llvm-test-suite/blob/intel/SYCL/DeviceLib/separate_compile_test.cpp

Such usage model caused the hit of the new static_assert (added in this PR).
It might be useful to add "-fsycl-host-only" option (similar to existing "-fsycl-device-only") in long terms if DPC++ keeps supporting separate device/host compilation.

In short terms it makes sense replacing "static_assert()" with '#pragma message()".

Signed-off-by: Vyacheslav N Klochkov <[email protected]>

…md_stl_static_assert

alexbatashev

LGTM

DenisBakhvalov

Changes in sycl/test/esimd/vadd.cpp LGTM

DenisBakhvalov · 2021-03-31T22:32:11Z

sycl/test/esimd/vadd.cpp

@@ -1,5 +1,4 @@
-// TODO ESIMD enable host device under -fsycl
-// RUN: %clangxx -I %sycl_include %s -o %t.out -lsycl
+// RUN: %clangxx -fsycl %s -o %t.out


Sorry for not catching it, but there is a regression for this test. BTW, I'm not sure how it slipped through the CI.
This test fails with:

InvalidFunctionCall: Unexpected llvm intrinsic: llvm.genx.local.id.v3i32 [Src: /iusers/dbakhval/github/llvm/llvm-spirv/lib/SPIRV/SPIRVWriter.cpp:2782 false ]

As of today, we still require -fsycl-explicit-simd flag for ESIMD code. I submitted #3457 to fix this.

BTW, I'm not sure how it slipped through the CI.

This casts shadow on CI validation and ESIMD feature validation in particular. @DenisBakhvalov, could you investigate why this regression happened, please? Could it be a conflict between commits?

…-patch [L0] Fixes potential overwrite in ZeEventPoolDesc.pNext

[L0] Fixes potential overwrite in ZeEventPoolDesc.pNext

v-klochkov requested a review from a team as a code owner September 21, 2020 05:35

v-klochkov requested review from againull, romanovvlad and alexbatashev September 21, 2020 05:35

alexbatashev previously approved these changes Sep 21, 2020

View reviewed changes

vladimirlaz reviewed Sep 21, 2020

View reviewed changes

v-klochkov dismissed alexbatashev’s stale review via f875ed0 September 21, 2020 17:38

alexbatashev previously approved these changes Sep 22, 2020

View reviewed changes

romanovvlad requested changes Sep 22, 2020

View reviewed changes

sycl/include/CL/sycl/stl.hpp Show resolved Hide resolved

v-klochkov dismissed alexbatashev’s stale review via a132f41 March 26, 2021 18:51

v-klochkov force-pushed the public_vklochkov_md_stl_static_assert branch from f875ed0 to a132f41 Compare March 26, 2021 18:51

v-klochkov force-pushed the public_vklochkov_md_stl_static_assert branch from a132f41 to 47e9ed2 Compare March 26, 2021 22:20

v-klochkov requested review from DenisBakhvalov and kbobrovs as code owners March 26, 2021 22:20

kbobrovs reviewed Mar 27, 2021

View reviewed changes

sycl/include/CL/sycl/stl.hpp Outdated Show resolved Hide resolved

alexbatashev reviewed Mar 29, 2021

View reviewed changes

v-klochkov added 2 commits March 29, 2021 11:14

Remove 'inline' keyword per reviewer's request, fix one LIT test

852c3f4

Signed-off-by: Vyacheslav N Klochkov <[email protected]>

Merge remote-tracking branch 'intel_llvm/sycl' into public_vklochkov_…

373bbb6

…md_stl_static_assert

kbobrovs previously approved these changes Mar 29, 2021

View reviewed changes

v-klochkov added 2 commits March 30, 2021 23:47

Turned the static_assert() into a warning/message

31f880a

Signed-off-by: Vyacheslav N Klochkov <[email protected]>

Merge remote-tracking branch 'intel_llvm/sycl' into public_vklochkov_…

41829de

…md_stl_static_assert

v-klochkov dismissed kbobrovs’s stale review via 41829de March 31, 2021 06:48

v-klochkov changed the title ~~[SYCL] Check that apps using sycl also use dynamic C++ RT even withou…~~ [SYCL] Warning if use sycl without dynamic C++ RT on Windows Mar 31, 2021

alexbatashev approved these changes Mar 31, 2021

View reviewed changes

romanovvlad approved these changes Mar 31, 2021

View reviewed changes

DenisBakhvalov approved these changes Mar 31, 2021

View reviewed changes

v-klochkov merged commit d4180f4 into intel:sycl Mar 31, 2021

v-klochkov deleted the public_vklochkov_md_stl_static_assert branch March 31, 2021 17:58

DenisBakhvalov mentioned this pull request Mar 31, 2021

[ESIMD] Fixed in-tree vadd test #3457

Merged

DenisBakhvalov reviewed Mar 31, 2021

View reviewed changes

kbenzie pushed a commit to kbenzie/intel-llvm that referenced this pull request Feb 17, 2025

Merge pull request intel#2501 from winstonzhang-intel/interrupt-based…

db491ab

…-patch [L0] Fixes potential overwrite in ZeEventPoolDesc.pNext

Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025

Merge pull request #2501 from winstonzhang-intel/interrupt-based-patch

6b7b44b

[L0] Fixes potential overwrite in ZeEventPoolDesc.pNext

		@@ -1,5 +1,3 @@
		// RUN: %clangxx %s -o %t1.out -lsycl -I %sycl_include

		@@ -1,4 +1,4 @@
		// RUN: %clangxx -fsyntax-only -Xclang -verify %s -I %sycl_include -Xclang -verify-ignore-unexpected=note,warning

	inline constexpr bool isMSVCDynamicCXXRuntime() {
	constexpr bool isMSVCDynamicCXXRuntime() {

[SYCL] Warning if use sycl without dynamic C++ RT on Windows #2501

[SYCL] Warning if use sycl without dynamic C++ RT on Windows #2501

Uh oh!

Conversation

v-klochkov commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romanovvlad commented Sep 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bader commented Feb 12, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kbobrovs left a comment

Choose a reason for hiding this comment

Uh oh!

v-klochkov commented Mar 30, 2021

Uh oh!

alexbatashev left a comment

Choose a reason for hiding this comment

Uh oh!

DenisBakhvalov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

v-klochkov commented Sep 21, 2020 •

edited

Loading