[SYCL] Remove kernel set id to fix kernel lookup #10551

KseniyaTikhomirova · 2023-07-25T14:02:46Z

No description provided.

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

AlexeySachkov · 2023-08-08T07:59:40Z

sycl/source/detail/program_manager/program_manager.cpp

+  }
+
+  std::lock_guard<std::mutex> KernelIDsGuard(m_KernelIDsMutex);
+  // TODO: There may be cases with sycl::program class usage in source code


sycl::program class doesn't exists anymore in our implementation, I doubt that this comment should be re-populated throughout the codebase

comment introduced in 2019 and haven't been fixed for years (9095749). Removing comment as outdated

AlexeySachkov · 2023-08-08T08:00:45Z

sycl/source/detail/program_manager/program_manager.cpp

+  std::vector<pi_device_binary> RawImgs(std::distance(ItBegin, ItEnd));
+  auto It = ItBegin;
+  for (unsigned I = 0; It != ItEnd; ++It, ++I)
+    RawImgs[I] = const_cast<pi_device_binary>(&It->second->getRawData());


const_cast? Can we leave without it?

this const cast is not introduced by my change, just moved.
The method that is used here has const return value and const itself:
const pi_device_binary_struct &getRawData(). Then we are getting pointer to that value that has type const pi_device_binary_struct*.

piextDeviceSelectBinary where array is passed to accepts non const data that leads to the necessity of this cast. We could remove const from getRawData return value but I prefer to do cast explicitly only where it is needed and keep it const.
What do you think?

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

sergey-semenov

LGTM overall, mostly minor comments.

sycl/source/detail/memory_manager.cpp

sycl/source/detail/program_manager/program_manager.hpp

sergey-semenov · 2023-08-08T13:10:54Z

sycl/source/detail/program_manager/program_manager.hpp

+  /// Keeps images without entry info.
+  /// Such images are assumed to contain all kernel associated with the module.
+  /// Access must be guarded by the \ref m_KernelIDsMutex mutex.
+  std::unordered_set<RTDeviceBinaryImage *> m_UniversalKernelSet;


Do we still need this? I think this is just left from when we didn't have kernel entry information embedded in the fat binary, which, after several ABI breaking windows, shouldn't be something we support anymore.

You mentioned that this is still used by some of the tests. Assuming it's done there just for convenience, can we rewrite them to make use of the "proper" path and get rid of this universal kernel set logic altogether?

adding filter to throw out useless device image generated when no device code is present in application, will be fixed

This comment is not blocking this review and can be applied in a follow-up PR.

sycl/source/detail/program_manager/program_manager.cpp

KseniyaTikhomirova · 2023-08-10T13:44:38Z

fixed reviewer's comments. converting to draft for a while to double check possible memory leaks of images

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

sergey-semenov · 2023-08-14T10:42:22Z

sycl/source/detail/program_manager/program_manager.hpp


  /// Caches all exported symbols to allow faster lookup when excluding these
  // from kernel bundles.
  /// Access must be guarded by the m_KernelIDsMutex mutex.
  std::unordered_set<std::string> m_ExportedSymbols;

+  /// Keeps all device images we are refering to during program lifetime. Used
+  /// for proper cleanup.
+  std::unordered_set<RTDeviceBinaryImageUPtr> m_DeviceImages;


Non-blocking: why is this an unordered_set instead of a plain vector? Do we ever expect a duplicate image to be inserted here?

sergey-semenov · 2023-08-14T10:44:24Z

sycl/test-e2e/ProgramManager/uneven_kernel_split.cpp

@@ -0,0 +1,45 @@
+// REQUIRES: gpu, TEMPORARY_DISABLED


This is disabled due to a test issue, agreed to have this fixed and enabled in a follow-up PR.

sergey-semenov · 2023-08-14T10:48:23Z

sycl/source/detail/program_manager/program_manager.cpp


+  if (m_UniversalKernelSet.size())


Suggested change

if (m_UniversalKernelSet.size())

if (!m_UniversalKernelSet.empty())

Nitpick

sergey-semenov · 2023-08-14T10:49:49Z

sycl/source/detail/program_manager/program_manager.cpp

+  if (m_UniversalKernelSet.size())
+    return getDeviceImage(m_UniversalKernelSet, Context, Device,
+                          JITCompilationIsRequired);
+  else


Suggested change

else

Non-blocking, no else after return.

sergey-semenov · 2023-08-14T10:52:48Z

Since this PR is urgent and the only remaining comments are non-blocking, I'm merging it as is. Please, address the remaining comments shortly in a follow-up PR.

The intel/llvm/pull/10551 has been merged, so the build should succeed and produce working binary.

The intel/llvm/pull/10551 has been merged, so the build should succeed and produce working binary. The intel/llvm project has transitioned from sycl-nightly tags to nightly tags instead.

The intel/llvm/pull/10551 has been merged, so the build should succeed and produce working binary. The intel/llvm project has transitioned from sycl-nightly/YYYYMMDD tags to nightly-YYYY-MM-DD tags instead.

The intel/llvm/pull/10551 has been merged, so the build should succeed and produce working binary. The intel/llvm project has transitioned from sycl-nightly/YYYYMMDD tags to nightly-YYYY-MM-DD tags instead. The artifact of intel/llvm nightly build has also changed the name and the structure. Adjusting the code for that.

KseniyaTikhomirova added 13 commits July 11, 2023 05:59

draft

6325f28

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

fix memory leak

04eb19d

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

return multimap

ab4d0ea

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

final

51a7656

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

add service kernels to search

19c6b14

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

draft

bd9abdb

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

add universal set back

43aa741

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

Merge branch 'sycl' into kernel_lookup_failure

9a87064

fix tests

0198534

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

fix gap

278002f

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

remove prints

9d40069

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

fix clang-format

c27e364

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

move general code to function

f82268a

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

KseniyaTikhomirova temporarily deployed to aws August 3, 2023 14:55 — with GitHub Actions Inactive

fix warning

1ed4b44

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

KseniyaTikhomirova temporarily deployed to aws August 3, 2023 15:32 — with GitHub Actions Inactive

KseniyaTikhomirova temporarily deployed to aws August 3, 2023 16:19 — with GitHub Actions Inactive

KseniyaTikhomirova added 2 commits August 7, 2023 02:52

fix mutex lock

2d59ce9

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

fix spv usage

db9dce0

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

KseniyaTikhomirova temporarily deployed to aws August 7, 2023 10:18 — with GitHub Actions Inactive

KseniyaTikhomirova temporarily deployed to aws August 7, 2023 11:18 — with GitHub Actions Inactive

KseniyaTikhomirova changed the title ~~Kernel lookup failure~~ [SYCL] Remove kernel set id to fix kernel lookup Aug 7, 2023

KseniyaTikhomirova marked this pull request as ready for review August 7, 2023 13:17

KseniyaTikhomirova requested a review from a team as a code owner August 7, 2023 13:17

KseniyaTikhomirova requested a review from sergey-semenov August 7, 2023 13:17

AlexeySachkov reviewed Aug 8, 2023

View reviewed changes

KseniyaTikhomirova added 2 commits August 9, 2023 05:06

add test as reported issue follow up

19c8e49

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

add empty lines at the eof

5ffb319

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

KseniyaTikhomirova requested a review from AlexeySachkov August 9, 2023 12:49

fix comment

dc81435

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

sergey-semenov reviewed Aug 9, 2023

View reviewed changes

KseniyaTikhomirova marked this pull request as draft August 10, 2023 13:44

KseniyaTikhomirova added 3 commits August 10, 2023 06:44

fix code review comments

49c1402

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

fix print text

774f390

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

add container to provide proper memory release for device image wrapper

2b219d6

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

KseniyaTikhomirova marked this pull request as ready for review August 10, 2023 16:46

KseniyaTikhomirova requested a review from sergey-semenov August 10, 2023 16:46

KseniyaTikhomirova added 5 commits August 10, 2023 09:47

disable test on cuda & hip. gen testing is enough

ddbcbde

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

disable new test

621517c

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

temporary restore universal kernel set

1164c7b

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

fix segfault

c4ec7fb

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

avoid conflict in images used by usit test

477c227

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

sergey-semenov approved these changes Aug 14, 2023

View reviewed changes

sergey-semenov merged commit e07deff into intel:sycl Aug 14, 2023

oleksandr-pavlyk added a commit to IntelPython/dpctl that referenced this pull request Aug 15, 2023

Use latest sycl bundle to build DPCTL

35221e6

The intel/llvm/pull/10551 has been merged, so the build should succeed and produce working binary.

oleksandr-pavlyk mentioned this pull request Aug 15, 2023

Use latest sycl bundle to build DPCTL IntelPython/dpctl#1346

Merged

6 tasks

	if (m_UniversalKernelSet.size())
	if (!m_UniversalKernelSet.empty())

[SYCL] Remove kernel set id to fix kernel lookup #10551

[SYCL] Remove kernel set id to fix kernel lookup #10551

Uh oh!

Conversation

KseniyaTikhomirova commented Jul 25, 2023

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!

sergey-semenov left a comment

Choose a reason for hiding this comment

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!

KseniyaTikhomirova commented Aug 10, 2023 • 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

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!

sergey-semenov commented Aug 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

KseniyaTikhomirova commented Aug 10, 2023 •

edited

Loading

sergey-semenov commented Aug 14, 2023 •

edited

Loading