[SYCL] Avoid ABI issues with SYCL RT

[alexbatashev]

Microsoft Visual C++ compiler has different stdlib variants for release and debug build types (flags /MD and /MDd). These versions are incompatible and mixing them causes unpredictable stability issues and crashes. To overcome mentioned problem, two SYCL DLL libraries must be generated: for debug and release modes. Clang driver on Windows must honor build type flags and choose appropriate runtime version.

Signed-off-by: Alexander Batashev alexander.batashev@intel.com

[asavonic]

Can a DLL built with /MD flag be used by a program built with /MT?

[alexbatashev]

Yes, it should work fine.
Actually, I think providing a library variant built with /MT (if that's what your're implying) may cause a lot more trouble. Let's say, we link SYCL RT DLL against CRT of version X. And user downloads pre-built compiler and runtime, develops an application and links it against SYCL RT and CRT of version Y. There are now 2 CRTs in app's address space. And they may be incompatible. Thus, I think providing only /MD version should be sufficient.

[asavonic]

I think providing a library variant built with /MT (if that's what your're implying)

I did not suggest that. The only question is whether /MT for a user program works with sycl.dll or not. If it works fine, then please add another RUN line with /MT to the tests. If it doesn't - then we should explicitly state this requirement or provide a solution for /MT.

[bader]

I don't think we should build two versions of SYCL runtime library. I consider this patch as a work-around for poor SYCL library ABI design.
We didn't pay much attention to the library ABI design and it .
I would prefer you to review binary interface and make it stable and portable instead of this change.

[v-klochkov]

@bader - Alexey, Do you suggest replacing all standard classes such as vector, etc with something else?
Those debug|release versions of C Runtime Libraries are linked when you use any of this impressive list of C++ header files: https://docs.microsoft.com/en-us/cpp/standard-library/cpp-standard-library-header-files?view=vs-2019

See, it is written here: https://docs.microsoft.com/en-us/cpp/c-runtime-library/crt-library-features?view=vs-2019

When you build a release version of your project, one of the basic C run-time libraries (libcmt.lib, msvcmrt.lib, msvcrt.lib) is linked by default, depending on the compiler option you choose (multithreaded, DLL, /clr). If you include one of the C++ Standard Library header files in your code, a C++ Standard Library will be linked in automatically by Visual C++ at compile time.

[v-klochkov]

Adding to my previous statement.
I believe the poor ABI is not caused by our library implementation, but by the SYCL SPEC.
SYCL SPEC makes us using those std::vector, std::string, etc
Here is extract from it:

namespace cl {
9 namespace sycl {
10
11 template < class T, class Alloc = std::allocator >
12 using vector_class = std::vector<T, Alloc>;
13
14 using string_class = std::string;
15
16 template<class R, class... ArgTypes>
17 using function_class = std::function<R(ArgTypes...)>;
18
19 using mutex_class = std::mutex;
20
21 template
22 using shared_ptr_class = std::shared_ptr;
23
24 template
25 using unique_ptr_class = std::unique_ptr;
26
27 template
28 using weak_ptr_class = std::weak_ptr;
29
30 template
31 using hash_class = std::hash;
32
33 using exception_ptr_class = std::exception_ptr;
34
35 } // sycl
36 } // cl

[bader]

Alexey, Do you suggest replacing all standard classes such as vector, etc with something else?

Yes.

SYCL SPEC makes us using those std::vector, std::string, etc

No, it's not. It requires SYCL implementation to define <...>_class classes, which can be aliases to standard library classes, but specification does not require this particular implementation.

Anyway we do not have to use SYCL API in the library interface. It's an implementation decision - how functionality is split between library and headers and interface is used between them. Using C++ classes in ABI is not recommended as C++ classes layout is implementation defined, so using different compilers to build headers and a library doesn't work in general case.

[v-klochkov]

Ok, I understood what you mean, and agree now that it would be great to avoid using std::vector args/returns of exported functions in sycl/source. That would let us have only 1 sycl.dll. Thank you.

[v-klochkov]

I suggest to have this as a temporary solution.
It can be reverted after the rework of abi is done.

Please also add /MT compilation test cases too.

[alexbatashev]

@v-klochkov I rebased onto current state (GitHub showed some conflicts in lit.cfg).

[romanovvlad]

It seems the patch introduces fail on windows, please, fix.

[alexbatashev]

@romanovvlad the regression exists even without my patch. We just don’t have any tests for clang-cl. @AGindinson is investigating this.

[AGindinson]

@romanovvlad the regression exists even without my patch. We just don’t have any tests for clang-cl. @AGindinson is investigating this.

I confirm that the regression is not inflicted by @alexbatashev's commit. The guilty commit is [21fa901], I will create a PR in a few hours.

[romanovvlad]

@romanovvlad the regression exists even without my patch. We just don’t have any tests for clang-cl. @AGindinson is investigating this.

I confirm that the regression is not inflicted by @alexbatashev's commit. The guilty commit is [21fa901], I will create a PR in a few hours.

Ok, let's wait for the problem to be fixed.

[alexbatashev]

@AGindinson this didn't work for me.

[alexbatashev]

All issues should be fixed by #734

UPD: I confirm that #734 fully resolves the problem.