Skip to content

[SYCL] Add implementation of kernel_bundle. Part 3 #3375

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

Merged

Conversation

romanovvlad
Copy link
Contributor

@romanovvlad romanovvlad commented Mar 18, 2021

The patch adds

  1. Implementation of more kernel_bundle APIs.
  2. Support of kernel_bundles in handler

The patch duplicates some code in program manager in order to support
compilation for multiple devices. The refactoring of program manager
will be done in a separate patch.

Error handling will be added in a separate patch.

// If there were uses of set_specialization_constant build the kernel_bundle
std::shared_ptr<detail::kernel_bundle_impl> KernelBundleImpPtr =
getOrInsertHandlerKernelBundle(/*Insert=*/false);
if (KernelBundleImpPtr) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The kernel_bundle is used if there are calls to use_kernel_bundle or set_specialization_constants only.

@romanovvlad romanovvlad force-pushed the private/vromanov/KernelBundlesImpl6 branch from 03cf48c to 3a05631 Compare March 22, 2021 18:35
@@ -62,7 +163,9 @@ class CG {
FILL_USM = 11,
PREFETCH_USM = 12,
CODEPLAY_INTEROP_TASK = 13,
CODEPLAY_HOST_TASK = 14
CODEPLAY_HOST_TASK = 14,
KERNEL_V1 =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably it makes more sense to not have distinct enum value and hide manipulations with version in setType, getType, setVersion, getVersion methods.

@romanovvlad romanovvlad marked this pull request as ready for review March 25, 2021 18:03
@romanovvlad romanovvlad requested review from smaslov-intel and a team as code owners March 25, 2021 18:03
Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

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

Reviewed up to program manager (including).

Copy link
Contributor

@alexbatashev alexbatashev left a comment

Choose a reason for hiding this comment

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

LGTM if the comments above are resolved

@romanovvlad romanovvlad removed the request for review from smaslov-intel March 27, 2021 21:16
Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@kbobrovs kbobrovs left a comment

Choose a reason for hiding this comment

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

LGTM

@romanovvlad
Copy link
Contributor Author

/summary:run

Comment on lines +42 to +91
// Periodically there is a need to extend handler and CG classes to hold more
// data(members) than it has now. But any modification of the layout of those
// classes is an ABI break. To have an ability to have more data the following
// approach is implemented:
//
// Those classes have a member - MSharedPtrStorage which is an std::vector of
// std::shared_ptr's and is supposed to hold reference counters of user
// provided shared_ptr's.
//
// The first element of this vector is reused to store a vector of additional
// members handler and CG need to have.
//
// These additional arguments are represented using "ExtendedMemberT" structure
// which has a pointer to an arbitrary value and an integer which is used to
// understand how the value the pointer points to should be interpreted.
//
// ======== ======== ========
// | | | | ... | | std::vector<std::shared_ptr<void>>
// ======== ======== ========
// || || ||
// || \/ \/
// || user user
// || data data
// \/
// ======== ======== ========
// | Type | | Type | ... | Type | std::vector<ExtendedMemberT>
// | | | | | |
// | Ptr | | Ptr | ... | Ptr |
// ======== ======== ========
//
// Prior to this change this vector was supposed to have user's values only, so
// it is not legal to expect that the first argument is a special one.
// Versioning is implemented to overcome this problem - if the first element of
// the MSharedPtrStorage is a pointer to the special vector then CGType value
// has version "1" encoded.
//
// The version of CG type is encoded in the highest byte of the value:
//
// 0x00000001 - CG type KERNEL version 0
// 0x01000001 - CG type KERNEL version 1
// /\
// ||
// The byte specifies the version
//
// A user of this vector should not expect that a specific data is stored at a
// specific position, but iterate over all looking for an ExtendedMemberT value
// with the desired type.
// This allows changing/extending the contents of this vector without changing
// the version.
//
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably should update ABI policy guide after this patch is merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will add in a separate PR.

@romanovvlad romanovvlad merged commit ae45333 into intel:sycl Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants