Skip to content

Conversation

@KseniyaTikhomirova
Copy link
Contributor

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

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
@KseniyaTikhomirova
Copy link
Contributor Author

@tahonermann, @dvrogozh, @asudarsa, @aelovikov-intel, @sergey-semenov, @bader, @againull, @YuriPlyakhin, @vinser52

FYI, published for review.

typename backend_traits<Backend>::template return_type<SYCLObjectT>;

namespace detail {
inline std::string_view get_backend_name(const backend &Backend) {
Copy link
Contributor

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.

Copy link
Contributor Author

@KseniyaTikhomirova KseniyaTikhomirova Nov 17, 2025

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

Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I will remove this function form public headers
  2. although I don't understand why inline detail fully defined in header function should have any specific ABI related handling. What case do we consider?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KseniyaTikhomirova, the concern with inline functions is that they might not be inlined. If the inline function definition is changed such that two translation units observe different definitions and those definitions are not inlined, then the program will contain two distinct definitions, one of which will be selected by the linker or loader and used to satisfy all non-inlined references.

@tahonermann tahonermann self-requested a review November 17, 2025 18:07
typename backend_traits<Backend>::template return_type<SYCLObjectT>;

namespace detail {
inline std::string_view get_backend_name(const backend &Backend) {
Copy link
Contributor

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 <kseniya.tikhomirova@intel.com>
aelovikov-intel added a commit to aelovikov-intel/llvm-project that referenced this pull request Nov 18, 2025
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
aelovikov-intel added a commit to aelovikov-intel/llvm-project that referenced this pull request Nov 18, 2025
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
aelovikov-intel added a commit to aelovikov-intel/llvm-project that referenced this pull request Nov 18, 2025
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 <kseniya.tikhomirova@intel.com>
Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
_LIBSYCL_BEGIN_NAMESPACE_SYCL
namespace detail {

const char *stringifyErrorCode(int32_t error) {
Copy link
Contributor

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.

Copy link
Contributor

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)

Copy link
Contributor Author

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

Copy link
Contributor Author

@KseniyaTikhomirova KseniyaTikhomirova Dec 5, 2025

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

Comment on lines 1 to 8
#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)
Copy link
Contributor

@aelovikov-intel aelovikov-intel Nov 20, 2025

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:

  1. Smart use of preprocessor in distributable header files (i.e., libsycl/include instead of libsycl/source/**).
  2. 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>()                                                       

Copy link
Contributor Author

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?

Copy link
Contributor

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:

  1. Smart use of preprocessor in distributable header files (i.e., libsycl/include instead of libsycl/source/**).
  2. 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.

Copy link
Contributor Author

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 <kseniya.tikhomirova@intel.com>
Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
Comment on lines 61 to 63
} else {
return EXIT_SUCCESS;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} else {
return EXIT_SUCCESS;
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines 37 to 38
std::cout << "No platforms found." << std::endl;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::cout << "No platforms found." << std::endl;
}
std::cout << "No platforms found." << std::endl;
return EXIT_SUCCESS;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

auto PlatformsView = detail::platform_impl::getPlatforms();
std::vector<platform> Platforms;
Platforms.reserve(PlatformsView.size());
for (size_t i = 0; i < PlatformsView.size(); i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


const char *exception::what() const noexcept { return MMessage->c_str(); }

bool exception::has_context() const noexcept { /*return (MContext != nullptr);*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool exception::has_context() const noexcept { /*return (MContext != nullptr);*/
bool exception::has_context() const noexcept {

Copy link
Contributor Author

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 <kseniya.tikhomirova@intel.com>
Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
@KseniyaTikhomirova
Copy link
Contributor Author

KseniyaTikhomirova commented Dec 8, 2025

I'd like to define naming style for the upstreaming code base.

Given:
General LLVM rule says:

In general, names should be in camel case (e.g. TextFileReader and isLValue())

and

As an exception, classes that mimic STL classes can have member names in STL’s style of lowercase words separated by underscores (e.g. begin(), push_back(), and empty()).

SYCL 2020 declare types in STL's style with snake case.

LLVM guide has no requirements about file names so I suggest:

  1. all file names under libsycl folder (.h, .cpp, .md, etc) will be in snake_case (no exceptions)
  2. SYCL API type names - follow SYCL2020 spec naming - snake_case
  3. all detail (excluding STL-like types) types to use CamelCase.

changes comparing to impl/llvm:
all API and impl related files are already in snake case but docs and tests use mixed style.
I suggest to use snake_case for all files in repo.

docs example:
https://github.com/intel/llvm/blob/sycl/sycl/doc/design/CompileTimeProperties.md
mixed style examples (see folder names as well)
https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/AddressCast/dynamic_address_cast.cpp
https://github.com/intel/llvm/blob/sycl/sycl/unittests/accessor/AccessorImplicitConversion.cpp
https://github.com/intel/llvm/blob/sycl/sycl/test/e2e_test_requirements/check_e2eexpr_logic.cpp

Apply CamelCase to our impl and service classes in snake_case, for example platform_impl should become PlatformImpl and so on:
example, see forward decl: https://github.com/intel/llvm/blob/sycl/sycl/source/detail/program_manager/program_manager.hpp#L76

@bader @tahonermann could you please provide your opinion:

  1. should we even align style or it is good to go with what we have in intel/llvm
  2. agree or not with proposed strategy

@bader
Copy link
Contributor

bader commented Dec 8, 2025

I'd like to define naming style for the upstreaming code base.

LLVM guide has no requirements about file names so I suggest:

  1. all file names under libsycl folder (.h, .cpp, .md, etc) will be in snake_case (no exceptions)
  2. SYCL API type names - follow SYCL2020 spec naming - snake_case
  3. all detail (excluding STL-like types) types to use CamelCase.

@KseniyaTikhomirova, please, create a libsycl/docs/CodingGuidelines.rst file and put your suggestion there. Let's separate this discussion from adding SYCL platform.

changes comparing to impl/llvm:

What is impl/llvm? Do you mean intel/llvm repository?

Copy link
Contributor

@tahonermann tahonermann left a 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.

: exception({EV, ECat}, WhatArg) {}
exception(int EV, const std::error_category &ECat)
: exception({EV, ECat}, "") {}

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

context related methods are omitted because context is not implemented. will be added once I add context class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. For ease of review though, it is helpful if the full class synopsis is present even if parts of it are commented out or implemented with FIXME placeholders. That way we don't have to compare the class definition against the SYCL specification in future reviews to look for potentially missed declarations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have received right the opposite comment from another reviewer when I left other methods commented out.
I want to suggest to leave it at it is for now. There are (and will) more things that I can't implement right here, right now (e.g. full list of device info, etc) and it will bring a mess into code base to track these gaps here. In the next PR for device impl I am adding high level list of missed items where this gap is already mentioned. So it shoulnd't be lost.
https://github.com/KseniyaTikhomirova/llvm-project/pull/3/files#diff-e122f118f7e566d443f20bc82a475cbd291333a03b17a2d44b7b9d134dc45d80R87

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not surprised that you've gotten conflicting review feedback. That's fine. At some point we'll need to audit to ensure that all required declarations are present anyway.

@Fznamznon Fznamznon added the SYCL https://registry.khronos.org/SYCL label Dec 11, 2025
Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
@KseniyaTikhomirova
Copy link
Contributor Author

I'd like to define naming style for the upstreaming code base.
LLVM guide has no requirements about file names so I suggest:

  1. all file names under libsycl folder (.h, .cpp, .md, etc) will be in snake_case (no exceptions)
  2. SYCL API type names - follow SYCL2020 spec naming - snake_case
  3. all detail (excluding STL-like types) types to use CamelCase.

@KseniyaTikhomirova, please, create a libsycl/docs/CodingGuidelines.rst file and put your suggestion there. Let's separate this discussion from adding SYCL platform.

changes comparing to impl/llvm:

What is impl/llvm? Do you mean intel/llvm repository?

thanks for suggestion, done #171867
@bader @tahonermann @AlexeySachkov @sergey-semenov @vinser52 it would be nice to get your opinion. I am not able to add you as reviewers to llvm-project PRs so inviting directly. Thanks.

Copy link
Contributor

@tahonermann tahonermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few additional comments and some responses to previous ones.

Comment on lines 67 to 69
#ifdef __SYCL_DEVICE_ONLY__
(void)Obj;
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this function won't implement useful semantics when compiled for the device, can we at least have it trap at run-time instead of returning a valid object that doesn't satisfy the function postconditions? E.g., __builtin_unreachable()?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we at least have it trap at run-time instead of returning a valid object that doesn't satisfy the function postconditions? E.g., __builtin_unreachable()?

This automatically assumes, that the underlying device compiler (and any formats for representing device code) support such semantics. That is not the case for SPIR-V, as far as I know, for example.

What is the reason for having the macro in the first place here? I can't imagine hash APIs being used from device code without violating some other restrictions or simply being a UB.

As such, triggering a compilation/link failure through an unresolved symbol wouldn't be a bad idea. We can just leave this function as a declaration-only for device code as well.

Copy link
Contributor

@Fznamznon Fznamznon Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is not the case for SPIR-V, as far as I know, for example.

https://godbolt.org/z/qd8fGTMrG . Shouldn't we see llvm-spirv fail here then?

This automatically assumes, that the underlying device compiler (and any formats for representing device code) support such semantics.

That is still a good point though, we don't know about other targets like PTX.

What is the reason for having the macro in the first place here?

Not an expert in std headers implementation but it fails to compile https://godbolt.org/z/Eb94KMMed

Copy link
Contributor Author

@KseniyaTikhomirova KseniyaTikhomirova Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the macro completely for this code. the best case - we rely on compiler diagnostic, worst case - UB, since "implementations are not required to support device code that calls library functions from the C++ core language"

done in 002eeb3

Comment on lines +44 to +45
# define _LIBSYCL_DLL_LOCAL __attribute__((visibility("hidden")))
# define _LIBSYCL_EXPORT __attribute__((visibility("default")))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right; the C++11 attribute syntax is more restrictive in where it can be placed. Can we not use use the C++11 placement everywhere though? Or would that conflict with the syntax location required for Microsoft's __declspex(X) syntax? It would be great if we can standardize on the C++11 attribute placement requirements.

: exception({EV, ECat}, WhatArg) {}
exception(int EV, const std::error_category &ECat)
: exception({EV, ECat}, "") {}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. For ease of review though, it is helpful if the full class synopsis is present even if parts of it are commented out or implemented with FIXME placeholders. That way we don't have to compare the class definition against the SYCL specification in future reviews to look for potentially missed declarations.

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
Copy link
Contributor

@Fznamznon Fznamznon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test on this stage?
Also some dumb comments from a brief look

#endif // __SYCL2020_DEPRECATED

#if defined(_WIN32) && !defined(_DLL) && !defined(__SYCL_DEVICE_ONLY__)
// When built for use with the MSVC C++ standard library, libsycl requires
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to care about this now before we claim support for Windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now I don't use any OS-specific features in libsycl. So theoretically, once liboffload adds win support - base libsycl support on Win should be enabled automatically. (we will need some advanced handling of Win for shutdown but I want to keep this part for later upstreaming phases)
So I added this check from the very beginning.

return ImplUtils::createSyclObjFromImpl<SyclObject>(std::forward<From>(from));
}

// SYCL 2020 4.5.2. Common reference semantics (std::hash support)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// SYCL 2020 4.5.2. Common reference semantics (std::hash support)
// SYCL 2020 4.5.2. Common reference semantics (std::hash support).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

backend_mismatch = 14,
};

/// Constructs an error code using E and sycl_category()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why some comments are with /// but some are with //? Also some are with a full stop, others are not.
Perhaps there was an attempt to create documentation, but I think documentation comments are usually more informative.

Suggested change
/// Constructs an error code using E and sycl_category()
/// Constructs an error code using E and sycl_category().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I walked through the code and updated comments, added missing parts c90c2a7
unfortunately it is not always possible to add more informative comments (for example for getters) but at least it aligns the style.

};

/// Used as a container for a list of asynchronous exceptions
///
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
///

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

},
&Mapping);
// Now register all platforms and devices into the topologies
auto &OffloadTopologies = getOffloadTopologies();
Copy link
Contributor

@Fznamznon Fznamznon Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LLVM coding guidelines suggest using auto only when the type is obvious, i.e. cases like auto* a = dyn_cast<type>(b), will that be the case for libsycl?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I don't think it is important to show exact type here. it is pretty obvious from code that it is a container and we have exact element type shown 2 lines below.

ol_platform_backend_t OlBackend = OL_PLATFORM_BACKEND_UNKNOWN;
Res = call_nocheck(olGetPlatformInfo, Plat, OL_PLATFORM_INFO_BACKEND,
sizeof(OlBackend), &OlBackend);
// If error occures, ignore platform and continue iteration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// If error occures, ignore platform and continue iteration
// If error occurs, ignore platform and continue iteration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ol_platform_handle_t Plat = nullptr;
ol_result_t Res = call_nocheck(
olGetDeviceInfo, Dev, OL_DEVICE_INFO_PLATFORM, sizeof(Plat), &Plat);
// If error occures, ignore platform and continue iteration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// If error occures, ignore platform and continue iteration
// If error occurs, ignore platform and continue iteration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +52 to +53
// case OL_PLATFORM_BACKEND_LEVEL_ZERO:
// return backend::level_zero;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there plans for level 0?
Perhaps it makes sense to leave a comment saying coming soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I expected this PR with L0 support to be merged much faster #158900 but it is still on review. Once it's merged these lines much be uncommented. May be I will be able to uncomment them before the merge of my PR.

Signed-off-by: Tikhomirova, Kseniya <kseniya.tikhomirova@intel.com>
@KseniyaTikhomirova
Copy link
Contributor Author

Can we add a test on this stage? Also some dumb comments from a brief look

I'd like to but we don't have CI ready.
With switch to liboffload I have to build all stack: LLVM, openmp, liboffload and only then libsycl so github-hosted runners are not able to deal with a huge build and run out of memory.
we are actually adding our runner and libsycl build on it llvm/llvm-zorg#668 but it isn't merged yet.
meanwhile I am working on LIT configs for libsycl to add check-sycl. Once CI runner is added and I am ready with configs I will create PR with basic test for this and device (next PR) functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

SYCL https://registry.khronos.org/SYCL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants