-
Notifications
You must be signed in to change notification settings - Fork 0
[SYCL] Add platform enumeration and info query using liboffload #1
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
Conversation
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]>
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]>
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]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
|
This PR is cut a little bit comparing to the one I planned for UR.
In the end this PR:
I believe this PR is ready for internal review: @tahonermann, @dvrogozh, @asudarsa, @aelovikov-intel, @sergey-semenov, @bader, @againull, @YuriPlyakhin |
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 had a brief look with some inline comments as I went. I think the high-level question we need to ask right now is this:
- Explicitly define naming convention (unlike the zoo in intel/llvm)
- Usage of LLVMSupport, at least under
src/ - Should the approach to
*_implclasses be revisited here - [Less important] TblGen vs. macros +
*.deffiles
Also + @vinser52
| // Helper function for extracting implementation from SYCL's interface objects. | ||
| // Note! This function relies on the fact that all SYCL interface classes | ||
| // contain "impl" field that points to implementation object. "impl" field | ||
| // should be accessible from this function. |
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 really like us to re-implement this in intel/llvm once our next ABI Breaking period opens (seems to be about a week away?) to have a single template base class abstracting this functionality away.
I also wonder how much it's even needed. AFAIK, those *_impl classes serve two main purposes:
- Backward ABI compatibility
- SYCL's common reference semantics
I'd argue that the ABI compatibility might not be necessary given that we plan to have versioning of the namespace and make actual use of 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.
@aelovikov-intel I agree with the idea of the base class.
Regarding the second part, if we get rid of pimpl pattern, it might cause ABI updates to occur much more frequently.
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.
Having a base class makes sense to me too. The pimpl idiom maps cleanly onto common reference semantics though, so I wonder how you envision implementing that without 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.
class sycl::device {
public:
device(const device &) = default;
device(device &&) = default;
device &operator=(const device &) = default;
device &operator=(device &&) = default;
inline friend operator==(..) = default;
private:
ol_device_handle_t ol_device;
};would satisfy the common reference semantics, wouldn't it? As such, for a device the only benefit of having device_impl would be for "caching" of things that aren't mapped directly to a liboffload API (e.g., sub-devices, maybe?).
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.
Regarding the second part, if we get rid of pimpl pattern, it might cause ABI updates to occur much more frequently.
inline namespace versioning is done at the outer namespace level, so once one class (including anything under sycl::detail::) needs an update, we provide an update in that minor release, 6 months cadence in upstream, IIUC.
As such, I don't think (or maybe hope?) the actual increase in frequency for the project as a whole would be significant.
| private: | ||
| // Exceptions must be noexcept copy constructible, so cannot use std::string | ||
| // directly. | ||
| // TODO: std::string will be converted to ABI neutral later. |
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 don't think "ABI neutral" should be upstreamed at all. And if it should, then everything should be STL-agnostic at the ABI boundary.
Also, any idea how std::exception deals with that (e.g., in libcxx)?
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.
Some time ago I asked Greg if we should support both ABIs and the answer was: "Yes, I think we want to upstream the CXX11_ABI stuff." So we do want to support it.
regarding libcxx I will check
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.
@gmlueck , why do we need that? AFAIK, upstream PyTorch has finally moved to C++11 ABI, e.g.:
- https://discuss.pytorch.org/t/why-is-libtorch-from-pip-not-build-with-cxx11-abi/207765/2
- Nightly pytorch wheel for prerelease version 2.6 is build with C++11 ABI on, at least for CPU pytorch/pytorch#143962 (comment)
and nobody is using the ancient pre-C++11 one.
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.
Are you asking about what we should upstream, or are you asking what we should support in our downstream "intel/llvm" repo? If we do not upstream the ABI neutral stuff, but we keep it in downstream, won't that be a pain whenever we pull from upstream?
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.
Emm, yes?.. But that's still a terrible argument for upstreaming that.
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 there are still customers that need this functionality, why wouldn't we upstream it? If there are no customers that want it, then should we propose to remove it also from "intel/llvm"?
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 whole solution was a quick hack. The root cause of the problem is the dependency of our ABI to a particular STL. Even if we want to support this usage scenario, the proper fix would be to avoid any STL data types crossing ABI boundary, not just std::string.
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 it's better to focus on the current and future usages rather than hold on and support older and outdated ones. Those who needs older support can still use those older releases which have it. It's actually important to encourage users to switch to newer stuff. When you are working on upstreaming changes, above becomes a rule as it's rarely of interest of community what were historic customized usages in the closed source solution.
bader
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.
LGTM in general.
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]>
| // Helper function for extracting implementation from SYCL's interface objects. | ||
| // Note! This function relies on the fact that all SYCL interface classes | ||
| // contain "impl" field that points to implementation object. "impl" field | ||
| // should be accessible from this function. |
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.
@aelovikov-intel I agree with the idea of the base class.
Regarding the second part, if we get rid of pimpl pattern, it might cause ABI updates to occur much more frequently.
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]>
|
just for me to summarize the keys comments left:
|
|
Another question: do we handle somewhere the list of exported symbols? Today, it is hard to control which symbols are exported by the library. |
not yet. #1 (comment) |
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]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
|
I either fixed or replied to all comments. So I think this PR is ready for the second round of review, thanks. |
bader
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.
Still LGTM :).
|
|
||
| // Derive from std::exception so uncaught exceptions are printed in c++ default | ||
| // exception handler. | ||
| // Virtual inheritance is mandated by SYCL2020. |
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.
| // Virtual inheritance is mandated by SYCL2020. | |
| // Virtual inheritance is mandated by SYCL 2020. |
Signed-off-by: Tikhomirova, Kseniya <[email protected]>
|
this PR is public now llvm#166927 |
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.