-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[SYCL] Add platform enumeration and info query using liboffload #166927
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[SYCL] Add platform enumeration and info query using liboffload #166927
Conversation
This is part of the SYCL support upstreaming effort. The relevant RFCs can be found here: https://discourse.llvm.org/t/rfc-add-full-support-for-the-sycl-programming-model/74080 https://discourse.llvm.org/t/rfc-sycl-runtime-upstreaming/74479 The SYCL runtime is device-agnostic and uses liboffload for offloading to GPU. This commit adds a dependency on liboffload, implementation of platform::get_platforms, platform::get_backend and platform::get_info methods, initial implementation of sycl-ls tool for manual testing of added functionality. Plan for next PR: device/context impl, rest of platform test infrastructure (depends on L0 liboffload plugin CI, our effort is joined) ABI tests
|
@tahonermann, @dvrogozh, @asudarsa, @aelovikov-intel, @sergey-semenov, @bader, @againull, @YuriPlyakhin, @vinser52 FYI, published for review. |
| // Exceptions must be noexcept copy constructible, so cannot use std::string | ||
| // directly. | ||
| std::shared_ptr<std::string> MMessage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great opportunity to start conversation with libcxx developers (@ldionne to start that, I think) on how/if we can share some of their implementation details. __libcpp_refstring would be perfect here.
Another potential opportunity (in a not so distant future PR) would be related to the implementation of std::shared_ptr. In a nutshell, some of the SYCL objects would probably be implemented (simplified) like this:
class event_impl; // defined inside libsycl.so, not exposed to the public headers
class event {
public:
/* ... */
private:
std::shared_ptr<event_impl> pImpl;
};Ideally, we'd want to inherit event_impl from std::enable_shared_from_this. The problem we saw is that inheritance by itself slowed down construction of event_impl because even if know that we always create them via make_shared<event_iml> the implementation still had to initialize the std::enable_shared_from_this subobject with atomic operations resulting in a measurable overhead. If we could somehow use most the libc++'s implementation of these but with a way to communicate extra guarantees of how those objects are created and not to pay the price of these initialization atomics, that would be great, but I'm not sure if that's possible/worth maintenance efforts.
| typename backend_traits<Backend>::template return_type<SYCLObjectT>; | ||
|
|
||
| namespace detail { | ||
| inline std::string_view get_backend_name(const backend &Backend) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to use something like _LIBCPP_HIDE_FROM_ABI here, if I understand the idea behind it correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave questions about ABI till the time I will add ABI tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, in intel/llvm this function is defined in headers only because we want to use it in the sycl-ls tool so the tool is always aligned on "known" backend with the SYCL RT.
If this intent still stands, I would add a comment about it, or otherwise this function should not exist here at all, because we don't use it anywhere else in the headers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is still used in sycl-ls.
added tiny comment
b15b6c0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend addressing ABI exposures as soon as possible. The code should be designed around ABI concerns if a stable ABI is to be supported. Delay could make handling ABI more complicated later.
| typename backend_traits<Backend>::template return_type<SYCLObjectT>; | ||
|
|
||
| namespace detail { | ||
| inline std::string_view get_backend_name(const backend &Backend) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, in intel/llvm this function is defined in headers only because we want to use it in the sycl-ls tool so the tool is always aligned on "known" backend with the SYCL RT.
If this intent still stands, I would add a comment about it, or otherwise this function should not exist here at all, because we don't use it anywhere else in the headers
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Various offload APIs `olGet*Info` are essentially untyped because they "return" value via `void *PropValue` output parameter. However, for C++ consumers (e.g., [SYCL][1] in llvm#166927) it would be beneficial if we could recover that type information. Before this PR it was only encoded in the comments near corresponding information info descriptors, e.g., ```c++ /////////////////////////////////////////////////////////////////////////////// /// @brief Supported event info. typedef enum ol_event_info_t { /// [ol_queue_handle_t] The handle of the queue associated with the device. OL_EVENT_INFO_QUEUE = 0, /// [bool] True if and only if the event is complete. OL_EVENT_INFO_IS_COMPLETE = 1, /// @cond OL_EVENT_INFO_LAST = 2, OL_EVENT_INFO_FORCE_UINT32 = 0x7fffffff /// @endcond } ol_event_info_t; ``` so was imposible for consumers to recover programmatically. [1] https://github.com/llvm/llvm-project/blob/b22192afdcbda7441e7a8fe7cbc9a06903e9e6ea/libsycl/src/detail/platform_impl.hpp#L78-L90
Various offload APIs `olGet*Info` are essentially untyped because they "return" value via `void *PropValue` output parameter. However, for C++ consumers (e.g., [SYCL][1] in llvm#166927) it would be beneficial if we could recover that type information. Before this PR it was only encoded in the comments near corresponding information info descriptors, e.g., ```c++ /////////////////////////////////////////////////////////////////////////////// /// @brief Supported event info. typedef enum ol_event_info_t { /// [ol_queue_handle_t] The handle of the queue associated with the device. OL_EVENT_INFO_QUEUE = 0, /// [bool] True if and only if the event is complete. OL_EVENT_INFO_IS_COMPLETE = 1, /// @cond OL_EVENT_INFO_LAST = 2, OL_EVENT_INFO_FORCE_UINT32 = 0x7fffffff /// @endcond } ol_event_info_t; ``` so was imposible for consumers to recover programmatically. [1] https://github.com/llvm/llvm-project/blob/b22192afdcbda7441e7a8fe7cbc9a06903e9e6ea/libsycl/src/detail/platform_impl.hpp#L78-L90
Various offload APIs `olGet*Info` are essentially untyped because they "return" value via `void *PropValue` output parameter. However, for C++ consumers (e.g., SYCL in llvm#166927) it would be beneficial if we could recover that type information. Before this PR it was only encoded in the comments near corresponding information info descriptors, e.g., ```c++ /////////////////////////////////////////////////////////////////////////////// /// @brief Supported event info. typedef enum ol_event_info_t { /// [ol_queue_handle_t] The handle of the queue associated with the device. OL_EVENT_INFO_QUEUE = 0, /// [bool] True if and only if the event is complete. OL_EVENT_INFO_IS_COMPLETE = 1, /// @cond OL_EVENT_INFO_LAST = 2, OL_EVENT_INFO_FORCE_UINT32 = 0x7fffffff /// @endcond } ol_event_info_t; ``` and not accessible programmatically.
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
| _LIBSYCL_BEGIN_NAMESPACE_SYCL | ||
| namespace detail { | ||
|
|
||
| const char *stringifyErrorCode(int32_t error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this API should become an liboffload entry point. I'll add that to my TODO list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is allready API in liboffload. There are << operator for enum ol_errc_t
Check OffloadPrint.hpp (note this is tablegen generated file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kseniya to check if we can reuse it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update: can't reuse since we won't use llvmSupport lib for SYCl RT but operator returns llvm::raw_stream
| #ifndef __SYCL_PARAM_TRAITS_SPEC | ||
| static_assert(false, "__SYCL_PARAM_TRAITS_SPEC is required but not defined"); | ||
| #endif | ||
|
|
||
| // 4.6.2.4. Information descriptors | ||
| __SYCL_PARAM_TRAITS_SPEC(platform, version, std::string, OL_PLATFORM_INFO_VERSION) | ||
| __SYCL_PARAM_TRAITS_SPEC(platform, name, std::string, OL_PLATFORM_INFO_NAME) | ||
| __SYCL_PARAM_TRAITS_SPEC(platform, vendor, std::string, OL_PLATFORM_INFO_VENDOR_NAME) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subjective, but I'm not a fan of this approach for two reasons:
- Smart use of preprocessor in distributable header files (i.e.,
libsycl/includeinstead oflibsycl/source/**). OL_*are implementation details and don't need to be present in those distributable headers.
I think avoiding it doesn't result in too much code duplication: 823c765, but I understand that this is subjective.
Macro in exports can be avoided too, but that's probably too much magic:
// sycl.hpp
#define _EXPORT __attribute__((visibility("default")))
struct S {
template <typename> _EXPORT void foo();
};
// libsycl.so
template <typename> [[gnu::used]] _EXPORT void S::foo() {}
template _EXPORT void S::foo<char>(); // current approach
// "Clever" helper, needs `[[gnu::used]]` above.
template <typename... Ts> void instantiate_helper() {
(((void)(&S::foo<Ts>), ...));
}
static void instantiate() { instantiate_helper<int, float, double>(); }$ clang++ a.cpp -c -fvisibility=hidden -fvisibility-inlines-hidden -O0 ; nm a.o | c++filt
0000000000000000 W void S::foo<char>()
0000000000000000 W void S::foo<double>()
0000000000000000 W void S::foo<float>()
0000000000000000 W void S::foo<int>()
$ clang++ a.cpp -c -fvisibility=hidden -fvisibility-inlines-hidden -O3 ; nm a.o | c++filt
0000000000000000 W void foo<char>()
0000000000000000 W void foo<double>()
0000000000000000 W void foo<float>()
0000000000000000 W void foo<int>()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to ask other folks for opinion here. The benefit that .def file provides is necessity in update of only one place in the code to add info or property (we use the same approach there) if there is no special handling.
@sergey-semenov, @bader, @vinser52, @tahonermann, @AlexeySachkov do you have any preference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subjective, but I'm not a fan of this approach for two reasons:
- Smart use of preprocessor in distributable header files (i.e.,
libsycl/includeinstead oflibsycl/source/**).OL_*are implementation details and don't need to be present in those distributable headers.
I don't understand the problem with using pre-processor in distributed header files, but I agree that mapping from SYCL API to liboffload API should be done in the library source code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to apply Andrey's proposal. Please let me know if anyone has objections.
the reason is:
I wanted info descriptors declaration to be independent for SYCL obj in terms of header files (the opposite to what we have in https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/detail/info_desc_helpers.hpp).
With macro it means pretty low level of unification for helpers. Base class for info desc as proposed by Andrey gives opportunities to achieve the goal I wanted. Plus 3 people agreed that Offload RT codes should be hidden in SYCL RT that already weakens approach with macros.
I have started implementation of 'device' and there we will be able to check & see how it fits into our code base.
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
libsycl/tools/sycl-ls/sycl-ls.cpp
Outdated
| } else { | ||
| return EXIT_SUCCESS; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } else { | |
| return EXIT_SUCCESS; | |
| } | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| std::cout << "No platforms found." << std::endl; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| std::cout << "No platforms found." << std::endl; | |
| } | |
| std::cout << "No platforms found." << std::endl; | |
| return EXIT_SUCCESS; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
libsycl/src/platform.cpp
Outdated
| auto PlatformsView = detail::platform_impl::getPlatforms(); | ||
| std::vector<platform> Platforms; | ||
| Platforms.reserve(PlatformsView.size()); | ||
| for (size_t i = 0; i < PlatformsView.size(); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://llvm.org/docs/CodingStandards.html#use-range-based-for-loops-wherever-possible
I suppose we can use range-based for loop here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
libsycl/src/exception.cpp
Outdated
|
|
||
| const char *exception::what() const noexcept { return MMessage->c_str(); } | ||
|
|
||
| bool exception::has_context() const noexcept { /*return (MContext != nullptr);*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| bool exception::has_context() const noexcept { /*return (MContext != nullptr);*/ | |
| bool exception::has_context() const noexcept { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
|
I'd like to define naming style for the upstreaming code base. Given:
and
SYCL 2020 declare types in STL's style with snake case. LLVM guide has no requirements about file names so I suggest:
changes comparing to impl/llvm:
Apply CamelCase to our impl and service classes in snake_case, for example platform_impl should become PlatformImpl and so on: @bader @tahonermann could you please provide your opinion:
|
@KseniyaTikhomirova, please, create a libsycl/docs/CodingGuidelines.rst file and put your suggestion there. Let's separate this discussion from adding SYCL platform.
What is impl/llvm? Do you mean intel/llvm repository? |
tahonermann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got part of the way through the PR today. I'll review more tomorrow.
| if ((NOT CMAKE_MSVC_RUNTIME_LIBRARY AND uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG") | ||
| OR (CMAKE_MSVC_RUNTIME_LIBRARY STREQUAL "MultiThreadedDebugDLL")) | ||
| set(LIBSYCL_SHARED_OUTPUT_NAME "${LIBSYCL_SHARED_OUTPUT_NAME}d") | ||
| endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMAKE_BUILD_TYPE is only relevant for single-configuration CMake generators, so this won't have an impact for multi-configuration (IDE) generators like for msbuild. For multi-configuration generators, I think the result will be that release and debug builds will use the same name (but be placed in separate directories).
I also remain skeptical that changing the name of the DLL based on build configuration is desirable. I do see that there is one case of existing precedent in OpenMP, but that requires explicit opt-in by setting the OPENMP_MSVC_NAME_SCHEME variable at configuration time. If we want to support appending a debug differentiator to the DLL name, I think we should do similarly and introduce a LIBSYCL_MSVC_NAME_SCHEME variable.
Also, uppercase_CMAKE_BUILD_TYPE doesn't seem to be assigned in this file. Is it being supplied from elsewhere? Perhaps this should use a generator expression instead: $<UPPER_CASE:${CMAKE_BUILD_TYPE}>.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Tom, this code is just moved from src/CMakeLists.txt one layer above to be shared with sycl-ls setup. I think we have already discussed this strategy in "add libsycl" PR. We can reiterate that discussion but it is not a target for this PR.
| -DLLVM_INSTALL_UTILS=ON \ | ||
| -DCMAKE_INSTALL_PREFIX=$installprefix \ | ||
| -DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi;libsycl;libunwind" \ | ||
| -DLLVM_ENABLE_RUNTIMES="offload;openmp;libsycl" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is openmp needed here?
This looks like good cleanup otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is liboffload dependency. yes, it is needed.
| SYCL runtime is not tested and is not guaranteed to work on Windows because offloading runtime (liboffload) used by SYCL runtime doesn't currently support Windows. | ||
| The limitation to be revised once liboffload will add support for Windows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to constrain lines to 80 characters (though it isn't always possible in .rst files). I think this can be simplified too.
| SYCL runtime is not tested and is not guaranteed to work on Windows because offloading runtime (liboffload) used by SYCL runtime doesn't currently support Windows. | |
| The limitation to be revised once liboffload will add support for Windows. | |
| Libsycl is not currently supported on Windows because it depends on liboffload | |
| which doesn't currently support Windows. |
|
|
||
| _LIBSYCL_BEGIN_NAMESPACE_SYCL | ||
|
|
||
| // 4.1. Backends |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // 4.1. Backends | |
| // SYCL 2020 4.1. Backends |
| _LIBSYCL_BEGIN_NAMESPACE_SYCL | ||
|
|
||
| // 4.1. Backends | ||
| enum class backend : char { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why char as the underlying type? I suggest unsigned char to avoid issues with the signedness of char being implementation-defined.
| // Compiler automatically adds /MD or /MDd when -fsycl switch is used. | ||
| // The options /MD and /MDd that make the code to use dynamic runtime also | ||
| // define the _DLL macro. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is correct. I don't see code in upstream Clang that would handle this. How would the driver know to pass /MD vs /MDd if -fsycl is passed?
| "SYCL library is designed to work safely with dynamic C++ runtime." \ | ||
| "Please use /MD switch with sycl.dll, /MDd switch with sycld.dll, " \ | ||
| "or -fsycl switch to set C++ runtime automatically." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "SYCL library is designed to work safely with dynamic C++ runtime." \ | |
| "Please use /MD switch with sycl.dll, /MDd switch with sycld.dll, " \ | |
| "or -fsycl switch to set C++ runtime automatically." | |
| "Libsycl requires use of a DLL version of the MSVC RT library. " \ | |
| "Please use /MD to link with a release build of libsycl or /MDd to link" \ | |
| " with a debug build." |
| return ImplUtils::createSyclObjFromImpl<SyclObject>(std::forward<From>(from)); | ||
| } | ||
|
|
||
| // std::hash support (4.5.2. Common reference semantics) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // std::hash support (4.5.2. Common reference semantics) | |
| // SYCL 2020 4.5.2. Common reference semantics (std::hash support) |
|
|
||
| _LIBSYCL_BEGIN_NAMESPACE_SYCL | ||
|
|
||
| class context; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The context class doesn't appear to be used in this header and isn't specified to be declared by this header in the SYCL specification.
| : exception({EV, ECat}, WhatArg) {} | ||
| exception(int EV, const std::error_category &ECat) | ||
| : exception({EV, ECat}, "") {} | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constructors with a parameter list that starts with a context type are omitted. I assume that is intentional, but it would be helpful to note it in a comment.
get_context() is likewise omitted.
This is part of the SYCL support upstreaming effort. The relevant RFCs can be found here:
https://discourse.llvm.org/t/rfc-add-full-support-for-the-sycl-programming-model/74080 https://discourse.llvm.org/t/rfc-sycl-runtime-upstreaming/74479
The SYCL runtime is device-agnostic and uses liboffload for offloading to GPU. This commit adds a dependency on liboffload, implementation of platform::get_platforms, platform::get_backend and platform::get_info methods, initial implementation of sycl-ls tool for manual testing of added functionality.
Plan for next PR:
device/context impl, rest of platform
test infrastructure (depends on L0 liboffload plugin CI, our effort is joined) ABI tests