[SYCL] Implement INTEL feature class online_compiler #2930

v-klochkov · 2020-12-21T21:44:41Z

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

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

v-klochkov · 2020-12-21T21:47:04Z

This pull request implements the SPEC: #2913
Also, the initial changes for this feature were merged here: #2901

kbobrovs · 2020-12-21T22:05:06Z

sycl/source/detail/online_compiler/ocloc_api.h

@@ -0,0 +1,110 @@
+//===------- ocloc_api.h --------------------------------------------------===//


What is the reason to copy this header into SYCL rather than reusing the existing one?

The alternative solution requires pulling the whole https://github.com/intel/compute-runtime workspace just for the purpose of using this one ocloc_api.h header seems excessive. Do you agree?
Or did you suggest something else?

I think pulling just this header from https://github.com/intel/compute-runtime is enough. For example, SYCL uses OpenCL headers to build, and pulls in just the headers, not the entire repo.

You cannot pull one file from repo. Git does not provide such functionality. Using other tools is not safe.
For OpenCL headers we pull the whole workspace: https://github.com/KhronosGroup/OpenCL-Headers.git

I see. Looks like this is necessary evil (duplication of the source) then :(

sycl/source/detail/online_compiler/online_compiler.cpp

sycl/test/on-device/online_compiler/online_compiler_OpenCL.cpp

kbobrovs · 2020-12-21T23:00:50Z

sycl/test/on-device/online_compiler/online_compiler_common.hpp

+  }
+}
+
+template <cl::sycl::INTEL::source_language Lang>


Right, I'll remove this function.

Fixed in 392cc46

sycl/test/on-device/online_compiler/online_compiler_common.hpp

kbobrovs · 2020-12-21T23:05:48Z

sycl/test/on-device/online_compiler/online_compiler_common.hpp

+      else
+        std::cerr << "Unexpected exception: " << Msg << "\n";
+    }
+    assert(TestPassed && "Failed to throw an exception for wrong program");


same as above

Sorry, can you elaborate this comment please? The commend above seems unrelated to this code.

replied right above

Fixed in 392cc46

same suggestion as above

sycl/test/abi/sycl_symbols_linux.dump

sycl/source/detail/online_compiler/online_compiler.cpp

v-klochkov · 2020-12-22T01:41:23Z

@vladimirlaz , the test on Linux reported "libclangFEWrapper.so: cannot open shared object file: No such file or directory"
because ocloc uses that library if '-cmc' option is passed to it.
I believe libclangFEWrapper.so is part of ONEAPI package. Does CI need some fix? Please advise how to proceed with CM test

…ly ocloc_api.h from git Signed-off-by: Vyacheslav N Klochkov <[email protected]>

sycl/test/on-device/online_compiler/online_compiler_L0.cpp

vladimirlaz · 2020-12-22T11:49:50Z

@vladimirlaz , the test on Linux reported "libclangFEWrapper.so: cannot open shared object file: No such file or directory"
because ocloc uses that library if '-cmc' option is passed to it.
I believe libclangFEWrapper.so is part of ONEAPI package. Does CI need some fix? Please advise how to proceed with CM test

it looks like a gap in dependencies configuration (some libraries are missed for GPU RT or ocloc package used as dependency). @yamanwan could you please make sure that all libraries required for dependencies are available for testing.

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

AaronBallman · 2020-12-22T18:27:40Z

Just a drive-by question -- has this undergone any sort of security review? This sure smells like an easy attack vector given that the compiler has specific syntax for causing controlled crashes. (I recall Matt Godbolt saying he eventually gave up on trying to harden compiler explorer over exactly these sort of problems -- he spins up a VM that expects to get trashed by users and automatically resets itself if a watcher process notices the VM has gone bad.)

Some examples:
https://godbolt.org/z/hnhq35
https://godbolt.org/z/TPPnWq
https://godbolt.org/z/vvYEf3
https://godbolt.org/z/4WPzxb

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

kbobrovs · 2020-12-22T20:10:36Z

Just a drive-by question -- has this undergone any sort of security review? This sure smells like an easy attack vector given that the compiler has specific syntax for causing controlled crashes.

No specific security review was made. What particular security issue do you see - API, scenario?

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

keryell

Since the test here gives a nice way to program in OpenCL using the SYCL program, I think it worth the effort to simplify the code sample to the maximum, as it might be copied again and again forever in user code or tutorials...

keryell · 2020-12-22T23:28:15Z

sycl/include/CL/sycl/INTEL/online_compiler.hpp

-  return std::vector<byte>{};
-}
+__SYCL_EXPORT std::vector<byte> online_compiler<source_language::cm>::compile(
+    const std::string &src, const std::vector<std::string> &options);


I have the feeling that

Suggested change

const std::string &src, const std::vector<std::string> &options);

const std::string &src, const std::vector<std::string> &options = {});

would make the first overload useless.

I surely thought about doing that, but it doesn't work and the attempt to add the default argument, then remove the overload accepting only 'src' causes this error during SYCL build phase:

llvm/sycl/include/CL/sycl/INTEL/online_compiler.hpp:195:73: error: default argument specified in explicit specialization [ -fpermissive ]
const std::string &src, const std::vector< std::string > &options = {});

I see, because you have the main definition as

template <typename... Tys> std::vector<byte> compile(const std::string &src, const Tys &... args);

you cannot add this default parameter there either.
Actually I am even surprised that it is possible to specialize a variadic argument by a non variadic one...

But, at the end, why do you need to declare some super generic compile() that you do not really use, instead of having extensions just adding incrementally the overload they want?
I have the feeling that this specialization just adds some complexity for nothing valuable.
And the variadic template derail some simple implementations like:
https://godbolt.org/z/Yfo9bd

#include <iostream> #include <string> #include <vector> struct a { template <typename... Types> static void compile(Types... args) { std::cout << "the generic default" << std::endl; } static void compile(std::string arg, std::vector<std::string> args = {}) { std::cout << "single arg with vector" << std::endl; } }; int main() { // This one is captured by the generic default because a `const char*` is not a `std::string`, but do we need this at all? a::compile("default"); a::compile(std::string{}); a::compile(std::string{}, {"opt"}); a::compile("", {"opt"}); a::compile(std::string{}, {}); a::compile(std::string{}, {"opt"}); return 0; }

But, at the end, why do you need to declare some super generic compile() that you do not really use, instead of having extensions just adding incrementally the overload they want?
I have the feeling that this specialization just adds some complexity for nothing valuable.
And the variadic template derail some simple implementations like:
https://godbolt.org/z/Yfo9bd

@kbobrovs , is the one who wrote the feature SPEC, perhaps Konst can answer your question.

The assumption here is that different languages may have very different parameter sets for compile. Variadic templates provide the needed freedom. Concrete specializations fix variants used for particular language.

To avoid "derailing" mentioned above, we simply don't provide compile specialization pairs which conflict in that way.

Overloading is surely an option too, but some overloads will be unavailable for certain languages, which adds confusion. Isn't variadic template is ok C++ way to go when parameter list can be arbitrary?

Adding @Pennycook and @rolandschulz for possible comments.

sycl/include/CL/sycl/INTEL/online_compiler.hpp

sycl/test/on-device/online_compiler/online_compiler_common.hpp

v-klochkov · 2020-12-23T07:07:24Z

@vladimirlaz , the test on Linux reported "libclangFEWrapper.so: cannot open shared object file: No such file or directory"
because ocloc uses that library if '-cmc' option is passed to it.
I believe libclangFEWrapper.so is part of ONEAPI package. Does CI need some fix? Please advise how to proceed with CM test

it looks like a gap in dependencies configuration (some libraries are missed for GPU RT or ocloc package used as dependency). @yamanwan could you please make sure that all libraries required for dependencies are available for testing.

@yamanwan
The test on Windows failed because ocloc64.dll was not found, which is unexpected. ocloc64.dll is expected to be placed together with ocloc.exe. Can this be fixed in CI environment as well, please?

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

keryell · 2020-12-23T10:16:46Z

sycl/source/detail/online_compiler/ocloc_api.h

+  OCLOC_VERSION_1_0 = OCLOC_MAKE_VERSION(1, 0),     ///< version 1.0
+  OCLOC_VERSION_CURRENT = OCLOC_MAKE_VERSION(1, 0), ///< latest known version
+  OCLOC_VERSION_FORCE_UINT32 = 0x7fffffff
+} ocloc_version_t;


Looking at this code, perhaps it could be modernized upstream too if it is C++ by replacing OCLOC_MAKE_VERSION by a constexpr function in some nice namespace and the enum a C++ one without the typedef.
But this would be outside of this PR.

That could be done. The alternative to modernizing the file is keeping it as close to the original as possible to make it easier follow the changes in the original file.
I did only clang-format changes over the original.

To give more context for others: this file is copied from the underlying Gen driver source base. See the discussion above.

keryell · 2020-12-23T10:49:00Z

sycl/include/CL/sycl/INTEL/online_compiler.hpp

-  return std::vector<byte>{};
-}
+__SYCL_EXPORT std::vector<byte> online_compiler<source_language::cm>::compile(
+    const std::string &src, const std::vector<std::string> &options);


I see, because you have the main definition as

template <typename... Tys> std::vector<byte> compile(const std::string &src, const Tys &... args);

you cannot add this default parameter there either.
Actually I am even surprised that it is possible to specialize a variadic argument by a non variadic one...

But, at the end, why do you need to declare some super generic compile() that you do not really use, instead of having extensions just adding incrementally the overload they want?
I have the feeling that this specialization just adds some complexity for nothing valuable.
And the variadic template derail some simple implementations like:
https://godbolt.org/z/Yfo9bd

#include <iostream> #include <string> #include <vector> struct a { template <typename... Types> static void compile(Types... args) { std::cout << "the generic default" << std::endl; } static void compile(std::string arg, std::vector<std::string> args = {}) { std::cout << "single arg with vector" << std::endl; } }; int main() { // This one is captured by the generic default because a `const char*` is not a `std::string`, but do we need this at all? a::compile("default"); a::compile(std::string{}); a::compile(std::string{}, {"opt"}); a::compile("", {"opt"}); a::compile(std::string{}, {}); a::compile(std::string{}, {"opt"}); return 0; }

sycl/test/on-device/online_compiler/online_compiler_common.hpp

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

kbobrovs · 2020-12-23T18:33:01Z

sycl/test/on-device/online_compiler/online_compiler_common.hpp

+    assert(TestPassed &&
+           "Failed to throw an exception for unrecognized option");


Suggested change

assert(TestPassed &&

"Failed to throw an exception for unrecognized option");

kbobrovs · 2020-12-23T18:34:03Z

sycl/test/on-device/online_compiler/online_compiler_common.hpp

+    if (!TestPassed)
+      return 1;


Suggested change

if (!TestPassed)

return 1;

if (!TestPassed) {

std::cerr << "Failed to throw an expected exception for unrecognized option\n";

return 1;

}

kbobrovs · 2020-12-23T18:34:38Z

sycl/test/on-device/online_compiler/online_compiler_common.hpp

+      else
+        std::cerr << "Unexpected exception: " << Msg << "\n";
+    }
+    assert(TestPassed && "Failed to throw an exception for wrong program");


same suggestion as above

kbobrovs · 2020-12-23T18:36:35Z

sycl/source/detail/online_compiler/online_compiler.cpp

+#else
+    static const std::string OclocLibraryName = "libocloc.so";
+#endif
+    void *OclocLibrary = sycl::detail::pi::loadOsLibrary(OclocLibraryName);


kbobrovs · 2020-12-23T18:37:34Z

sycl/source/detail/online_compiler/online_compiler.cpp

+      std::memcpy(SpirV, Outputs[I], SpirVSize);
+    } else if (!strcmp(OutputNames[I], "stdout.log")) {
+      CompileLog = std::string(reinterpret_cast<const char *>(Outputs[I]));
+    }


kbobrovs

LGTM

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

…online_compiler

…s with exception.hpp Signed-off-by: Vyacheslav N Klochkov <[email protected]>

…ned off on Linux) Signed-off-by: Vyacheslav N Klochkov <[email protected]>

[SYCL] Implement INTEL feature class online_compiler

b1c348e

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

v-klochkov requested review from Pennycook, kbobrovs and smaslov-intel December 21, 2020 21:44

v-klochkov requested a review from a team as a code owner December 21, 2020 21:44

kbobrovs suggested changes Dec 21, 2020

View reviewed changes

kbobrovs reviewed Dec 21, 2020

View reviewed changes

sycl/test/abi/sycl_symbols_linux.dump Show resolved Hide resolved

sycl/source/detail/online_compiler/online_compiler.cpp Show resolved Hide resolved

Address all reviewer's comments except 2: a) test on host; b) pull on…

392cc46

…ly ocloc_api.h from git Signed-off-by: Vyacheslav N Klochkov <[email protected]>

smaslov-intel reviewed Dec 22, 2020

View reviewed changes

sycl/test/on-device/online_compiler/online_compiler_L0.cpp Show resolved Hide resolved

Fix the libocloc library version check

df799bd

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

Fix clang-format and fix linkage error on Widows

1d5671d

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

Run the LIT tests on HOST as well

e73322c

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

keryell reviewed Dec 22, 2020

View reviewed changes

Address the review comments from keryell

893852b

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

v-klochkov force-pushed the public_vklochkov_online_compiler branch from 5839689 to 893852b Compare December 23, 2020 07:36

v-klochkov requested a review from kbobrovs December 23, 2020 07:39

keryell reviewed Dec 23, 2020

View reviewed changes

More reviewer's comments addressed

c1382af

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

kbobrovs reviewed Dec 23, 2020

View reviewed changes

kbobrovs previously approved these changes Dec 24, 2020

View reviewed changes

Update the SYCL ABI test after all those additional fixes in this PR

2fd5d6f

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

v-klochkov dismissed kbobrovs’s stale review via 2fd5d6f December 24, 2020 20:39

v-klochkov added 2 commits December 24, 2020 12:43

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

d4a49e6

…online_compiler

Additional fixes in online_compile_error class after merging conflict…

f555c97

…s with exception.hpp Signed-off-by: Vyacheslav N Klochkov <[email protected]>

Enable the CM compilation test case for Windows only (temporarily tur…

ee66600

…ned off on Linux) Signed-off-by: Vyacheslav N Klochkov <[email protected]>

v-klochkov requested a review from romanovvlad December 24, 2020 21:22

v-klochkov force-pushed the public_vklochkov_online_compiler branch from f42dd53 to ee66600 Compare December 24, 2020 21:31

romanovvlad approved these changes Dec 29, 2020

View reviewed changes

romanovvlad merged commit 9112252 into intel:sycl Dec 29, 2020

v-klochkov deleted the public_vklochkov_online_compiler branch February 24, 2023 19:39

		@@ -0,0 +1,110 @@
		//===------- ocloc_api.h --------------------------------------------------===//

	const std::string &src, const std::vector<std::string> &options);
	const std::string &src, const std::vector<std::string> &options = {});

		assert(TestPassed &&
		"Failed to throw an exception for unrecognized option");

[SYCL] Implement INTEL feature class online_compiler #2930

[SYCL] Implement INTEL feature class online_compiler #2930

Uh oh!

Conversation

v-klochkov commented Dec 21, 2020

Uh oh!

v-klochkov commented Dec 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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

v-klochkov commented Dec 22, 2020

Uh oh!

Uh oh!

vladimirlaz commented Dec 22, 2020

Uh oh!

AaronBallman commented Dec 22, 2020

Uh oh!

kbobrovs commented Dec 22, 2020

Uh oh!

keryell 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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

v-klochkov Dec 22, 2020 •

edited

Loading

v-klochkov commented Dec 23, 2020 •

edited

Loading