[SYCL] Fix kernel program cache for multiple devices and refactor some unit tests #5017

maximdimakov · 2021-11-23T13:24:53Z

When multiple devices are used in the same context, the problem of piProgram double free is arised.
In program_manager::build() created program is shared with multiple devices and is cached in KernelProgramCache as many times as there are devices. When cache is destructed, the same program will be released for every device, but piProgramRetain was called only for one device, which leads to double free. Adding piProgramRetain for each device solves this problem.
Deleting kernel from cache after its release prevents double free in the case a kernel was created for a program that shared with multiple devices.
Unittest with emulating multiple gpu is added.
Signed-off-by: mdimakov [email protected]

Signed-off-by: mdimakov <[email protected]>

s-kanaev

The patch looks good.

@maximdimakov , could you, please, add regression test in unit-tests or intel/llvm-test-suite?

bader · 2021-12-03T08:05:49Z

/verify with intel/llvm-test-suite#594

bader

The patch, doesn't pass the test.

maximdimakov · 2021-12-05T08:07:38Z

There is problem in the test that it can't create multiple devices. I am investigating to it

s-kanaev · 2021-12-06T08:22:47Z

There is problem in the test that it can't create multiple devices. I am investigating to it

@maximdimakov , I suggest having a unit-test for this change as it doesn't require specific H/W or its features.

maximdimakov · 2021-12-15T07:05:53Z

@bader unit test passes checks

bader · 2021-12-15T09:14:19Z

@bader unit test passes checks

Good, but I'm expected you to fix llvm-test-suite tests you added for this patch.

maximdimakov · 2021-12-15T09:16:04Z

@bader I closed llvm-test-suite PR

bader · 2021-12-15T09:22:01Z

@bader I closed llvm-test-suite PR

Why? I don't think merging untested patch is a good idea.

maximdimakov · 2021-12-15T09:30:52Z

@bader I replaced test in test suite by unit test. I can emulate multiple devices with CreateMultipleRootDevices environment variable locally on machine, but Jenkins precommit machine can't. I think it is better to write unittest that doesn't depend on machine on which it runs.

Withdraw my request for changes and let Chris and Sergei to validate how well the patch is covered by the unittest.

s-kanaev · 2021-12-15T18:48:31Z

sycl/unittests/kernel-and-program/MultipleDevsCache.cpp

+
+class MultTestKernel {
+public:
+  void operator()(cl::sycl::item<1>){};


Should this be a const operator?

I removed this operator

s-kanaev · 2021-12-15T18:49:44Z

sycl/unittests/kernel-and-program/MultipleDevsCache.cpp

+  devices[0] = reinterpret_cast<pi_device>(1111);
+  devices[1] = reinterpret_cast<pi_device>(2222);


This should only be performed if devices is not nil.

And not just not nullptr. If I'm not mistaken, the maximum possible device entries is bound by num_entries so the subscripts should check to make sure they are less than num_entries. Though, in the case of mocks and unit tests, it's less clear to me where this value for num_entries comes from and if it is a problem to blindly set devices[1].

Would this test fail if run with the SYCL_DEVICE_FILTER set to a single device? (And, do we even care?)

I added check for nullptr and for num_entries.
piDevicesGet first time is called with num_entries=0 and not null num_devices. Ather the call num_entries takes value of num_devices. With second call num_devices parameter passes with nullptr, so we can create num_entries devices. I rewrite num_devices to 2, so when redefinedDevicesGet is called a second time RT thinks that we have two devices.
This test emulates two devices, so we don't care about real count of devices.

s-kanaev · 2021-12-15T18:51:30Z

sycl/unittests/kernel-and-program/MultipleDevsCache.cpp

+    EXPECT_EQ(KernelCache.size(), (size_t)2) << "Expect 2 kernels in cache";
+  }
+  // Cache is cleared here, check kernel release
+  EXPECT_EQ(KernelReleaseCounter, 3) << "Expect 3 piKernelRelease calls";


Please, add comment describing how number 3 was retrieved. Why is it expected and not 2 or four?

And why not test to see if KernelReleaseCounter is equal to the RetainCounter?

I added clarifying comments

And why not test to see if KernelReleaseCounter is equal to the RetainCounter?

AFAIR, there's some mem-leak test which checks for retain-release parity.
Although, I'm not sure it checks for kernels.

sycl/unittests/kernel-and-program/MultipleDevsCache.cpp

cperkinsintel

LGTM

s-kanaev

LGTM

bader · 2021-12-23T17:39:24Z

@cperkinsintel, ping

sycl/unittests/kernel-and-program/HelperKernelInfo.hpp

Apply LLVM's coding style rule - include as little as possible. https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible

cperkinsintel

LGTM

s-kanaev

LGTM

maximdimakov added 2 commits November 23, 2021 15:51

Fix kernel program cache for multiple devices

7bf586c

Signed-off-by: mdimakov <[email protected]>

Clang-format fix

a042881

maximdimakov requested a review from a team as a code owner November 23, 2021 13:24

maximdimakov requested review from cperkinsintel and s-kanaev November 23, 2021 13:24

s-kanaev previously approved these changes Nov 26, 2021

View reviewed changes

maximdimakov mentioned this pull request Dec 1, 2021

[SYCL] Add test for kernel program cache intel/llvm-test-suite#594

Closed

cperkinsintel previously approved these changes Dec 2, 2021

View reviewed changes

bader previously requested changes Dec 4, 2021

View reviewed changes

maximdimakov added 2 commits December 9, 2021 15:27

Merge remote-tracking branch 'origin/sycl' into fix_CP_cache

8458b5e

Add unittest for the fix

deaa97c

maximdimakov dismissed stale reviews from cperkinsintel and s-kanaev via deaa97c December 9, 2021 13:15

maximdimakov added 7 commits December 9, 2021 16:20

Clang-format fix

ab22f7c

Make test only for opencl backend

c248f7d

Clang-format fix

ca3c7dd

Fixed problem with kernel release

1aa4a39

Use size_t for reinterpret_cast

5674a12

Fix fail on windows

a718e3f

Clang-format

30db943

bader requested review from s-kanaev and cperkinsintel December 15, 2021 09:34

s-kanaev reviewed Dec 15, 2021

View reviewed changes

maximdimakov added 2 commits December 16, 2021 17:15

Addressed review comments

004c9d5

Clang-format fix

eb5abfb

s-kanaev reviewed Dec 17, 2021

View reviewed changes

sycl/unittests/kernel-and-program/MultipleDevsCache.cpp Outdated Show resolved Hide resolved

cperkinsintel previously approved these changes Dec 17, 2021

View reviewed changes

Add more clarifying comment

bc4be30

maximdimakov dismissed cperkinsintel’s stale review via bc4be30 December 19, 2021 19:13

Clang-format fix

a530172

s-kanaev previously approved these changes Dec 20, 2021

View reviewed changes

s-kanaev requested a review from cperkinsintel December 20, 2021 18:29

bader reviewed Dec 29, 2021

View reviewed changes

sycl/unittests/kernel-and-program/HelperKernelInfo.hpp Outdated Show resolved Hide resolved

maximdimakov and others added 4 commits January 10, 2022 14:59

Merge remote-tracking branch 'origin/sycl' into fix_CP_cache

034d92c

[SYCL][NFC] Don't include sycl.hpp from headers

b3f60a1

Apply LLVM's coding style rule - include as little as possible. https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible

[SYCL][NFC] Factor out empty kernel creation boilerplate

cb440dc

Refactor some tests

ed4811e

maximdimakov dismissed s-kanaev’s stale review via ed4811e January 10, 2022 15:43

maximdimakov changed the title ~~[SYCL] Fix kernel program cache for multiple devices~~ [SYCL] Fix kernel program cache for multiple devices and refactor some unit tests Jan 10, 2022

Clang-format fix

ad007d9

cperkinsintel approved these changes Jan 10, 2022

View reviewed changes

s-kanaev approved these changes Jan 11, 2022

View reviewed changes

bader merged commit 64c2d35 into intel:sycl Jan 11, 2022

maximdimakov deleted the fix_CP_cache branch March 24, 2022 13:59

		devices[0] = reinterpret_cast<pi_device>(1111);
		devices[1] = reinterpret_cast<pi_device>(2222);

[SYCL] Fix kernel program cache for multiple devices and refactor some unit tests #5017

[SYCL] Fix kernel program cache for multiple devices and refactor some unit tests #5017

Uh oh!

Conversation

maximdimakov commented Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-kanaev left a comment

Choose a reason for hiding this comment

Uh oh!

bader commented Dec 3, 2021

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

maximdimakov commented Dec 5, 2021

Uh oh!

s-kanaev commented Dec 6, 2021

Uh oh!

maximdimakov commented Dec 15, 2021

Uh oh!

bader commented Dec 15, 2021

Uh oh!

maximdimakov commented Dec 15, 2021

Uh oh!

bader commented Dec 15, 2021

Uh oh!

maximdimakov commented Dec 15, 2021

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!

cperkinsintel Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cperkinsintel left a comment

Choose a reason for hiding this comment

Uh oh!

s-kanaev left a comment

Choose a reason for hiding this comment

Uh oh!

bader commented Dec 23, 2021

Uh oh!

Uh oh!

cperkinsintel left a comment

Choose a reason for hiding this comment

Uh oh!

s-kanaev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maximdimakov commented Nov 23, 2021 •

edited

Loading

cperkinsintel Dec 15, 2021 •

edited

Loading