[SYCL] Rewrite tests that fail when usm_shared_allocations not supported #2 #12655

lbushi25 · 2024-02-07T21:54:26Z

Continuation of #12636.
Refer to it for a description.

github-actions · 2024-02-07T21:58:32Z

✅ With the latest revision this PR passed the C/C++ code formatter.

lbushi25 · 2024-02-09T19:33:00Z

@intel/llvm-reviewers-runtime

sommerlukas

Changes to kernel fusion tests LGTM.

uditagarwal97 · 2024-02-09T20:46:39Z

sycl/test-e2e/DiscardEvents/discard_events_mixed_calls.cpp

@@ -24,7 +24,7 @@
 // the tests, please check if they pass without the discard_events property, if
 // they don't pass then it's most likely a general issue unrelated to
 // discard_events.
-
+// REQUIRES: aspect-usm_shared_allocations


Am I right that we can't rewrite discard event test cases to use buffer & accessor instead because the latter introduces dependencies in the execution graph that this test case doesn't want?

That could be a reason but for me what convinced me to not do it is that many functions in this test have the USM string in their names. Therefore, with limited knowledge of how this test is supposed to work, I decided to leave it as is.

uditagarwal97 · 2024-02-09T21:22:10Z

sycl/test-e2e/GroupAlgorithm/root_group.cpp

@@ -76,44 +78,45 @@ void testRootGroupFunctions() {
      sycl::ext::oneapi::experimental::use_root_sync};

  constexpr int testCount = 10;
-  bool *testResults = sycl::malloc_shared<bool>(testCount, q);
+  sycl::buffer<bool> testResults_buf{sycl::range{testCount}};


I'd suggest using a consistent naming scheme. Since the code around this change uses camelCase, you could as well use that.

uditagarwal97 · 2024-02-09T21:27:04Z

sycl/test-e2e/InOrderEventsExt/set_external_event.cpp

@@ -15,20 +15,26 @@ int main() {
  sycl::queue Q1{Ctx, Dev, {sycl::property::queue::in_order{}}};
  sycl::queue Q2{Ctx, Dev, {sycl::property::queue::in_order{}}};

-  int *DevData = sycl::malloc_shared<int>(N, Dev, Ctx);
+  sycl::buffer<int> DevDatabuf{sycl::range{N}};


NIT: 'DevDatabuf' -> 'DevDataBuf'

uditagarwal97 · 2024-02-09T21:29:31Z

sycl/test-e2e/InOrderEventsExt/set_external_event.cpp

@@ -47,8 +53,5 @@ int main() {
    }
  }



IIUC, we should keep the 'free(HostData)' as it is still being malloc'd.

uditagarwal97 · 2024-02-09T21:42:03Z

sycl/test-e2e/KernelAndProgram/disable-caching.cpp

@@ -6,6 +6,7 @@
 // RUN: | FileCheck %s
 // RUN: env ZE_DEBUG=-6 SYCL_PI_TRACE=-1 %{run} %t.out \
 // RUN: | FileCheck %s --check-prefixes=CHECK-CACHE
+// REQUIRES: aspect-usm_shared_allocations


Can we rewite this test case to use buffer/accessors instead? I don't see how that change will alter the caching/eviction of kernels/programs.

I've changed it to use malloc_device because with buffer/accessor the test was failing due to some PI calls not showing up as expected.

uditagarwal97 · 2024-02-09T21:43:35Z

sycl/test-e2e/Reduction/reduction_internal.cpp

@@ -80,24 +80,22 @@ static void test(RedStorage &Storage, RangeTy Range) {
         cgh, Range, ext::oneapi::experimental::empty_properties_t{}, RedSycl,
         [=](auto Item, auto &Red) { Red.combine(T{1}); });
   }).wait();
-
-  auto *Result = malloc_shared<T>(1, q);
+  sycl::buffer<T> Result_buf{sycl::range{1}};


Same suggestion of using a consistent naming scheme.

uditagarwal97 · 2024-02-09T21:46:56Z

sycl/test-e2e/Reduction/reduction_range_item.cpp

@@ -3,7 +3,7 @@

 // Windows doesn't yet have full shutdown().
 // UNSUPPORTED: ze_debug && windows
-
+// REQUIRES: aspect-usm_shared_allocations


can we rewite this test using buffers intead?

uditagarwal97 · 2024-02-09T21:49:49Z

sycl/test-e2e/Reduction/reduction_span.cpp

@@ -3,7 +3,7 @@

 // Windows doesn't yet have full shutdown().
 // UNSUPPORTED: ze_debug && windows
-
+// REQUIRES: aspect-usm_shared_allocations


Same here. Can we rewrite it using buffers instead?

I think we do need some USM allocation for the span version of the reduction. However, using device allocations might make this test supported on a broader range of devices.

uditagarwal97 · 2024-02-09T21:50:04Z

sycl/test-e2e/Reduction/reduction_span_pack.cpp

@@ -6,7 +6,7 @@

 // Windows doesn't yet have full shutdown().
 // UNSUPPORTED: ze_debug && windows
-
+// REQUIRES: aspect-usm_shared_allocations


These tests that involve spans seem unfriendly to buffers/accessor as there are no span constructors that accept accessors as arguments. I could probably use the get_ptr method on an accessor and pass that pointer to the span but I've got a feeling that's probably undefined behavior, feel free to correct me though! I'll see if i can make them work instead with @aelovikov-intel's suggestion to use malloc_device instead.

lbushi25 · 2024-02-09T22:45:45Z

@uditagarwal97 Will change the names to camel case as you suggested.
Regarding the request to write some tests using buffer/accessors, I have tried implementing all the tests that you singled out using buffers/accessors but they were failing with very vague error reporting so I just decided to play it safe. I will try again to see if I've missed some test that should be doable with buffers/accessors but if a simple approach doesn't work, I won't try again since I don't want to potentially change what the test is trying to verify.

uditagarwal97 · 2024-02-09T23:47:24Z

@uditagarwal97 Will change the names to camel case as you suggested. Regarding the request to write some tests using buffer/accessors, I have tried implementing all the tests that you singled out using buffers/accessors but they were failing with very vague error reporting so I just decided to play it safe. I will try again to see if I've missed some test that should be doable with buffers/accessors but if a simple approach doesn't work, I won't try again since I don't want to potentially change what the test is trying to verify.

Sure. You can share with me the test case implementation with buffer/accessors that causes errors.

lbushi25 · 2024-02-09T23:56:36Z

@uditagarwal97 Will change the names to camel case as you suggested. Regarding the request to write some tests using buffer/accessors, I have tried implementing all the tests that you singled out using buffers/accessors but they were failing with very vague error reporting so I just decided to play it safe. I will try again to see if I've missed some test that should be doable with buffers/accessors but if a simple approach doesn't work, I won't try again since I don't want to potentially change what the test is trying to verify.

Sure. You can share with me the test case implementation with buffer/accessors that causes errors.

Have a look at my changes, with Andrei's suggestion of using malloc_device we get 100% test coverage just like buffers/accessors except that the test itself is simpler than buffer/accessor so I recommend we stick to this instead.

cperkinsintel

LGTM

Any thoughts about what to do moving forward as more tests get written? malloc_shared and friends are popular, seems likely to be reintroduced without the aspect check. I guess "constant vigilance" ?

lbushi25 · 2024-02-12T16:46:59Z

LGTM

Any thoughts about what to do moving forward as more tests get written? malloc_shared and friends are popular, seems likely to be reintroduced without the aspect check. I guess "constant vigilance" ?

From what I've seen so far, malloc_shared seems to be the only problematic one and hopefully people will steer away of this after facing these errors. I also think we should not have e2e tests run on devices that do not support malloc_shared. If we want to test for malloc_shared failure specifically, we can have some unit tests on mock devices.

lbushi25 · 2024-02-12T17:50:55Z

@uditagarwal97 Have a look when you have some time please!

uditagarwal97

LGTM!

lbushi25 and others added 7 commits January 18, 2024 16:15

Throw when USM not supported by Device

bee45c3

Merge branch 'intel:sycl' into sycl

d11d72a

Merge branch 'intel:sycl' into sycl

4045d77

Merge branch 'intel:sycl' into sycl

8c9d3d2

Merge branch 'intel:sycl' into sycl

4745d35

Rewrite tests

f3b90a4

Remove rogue changes

86e9144

lbushi25 temporarily deployed to WindowsCILock February 7, 2024 22:27 — with GitHub Actions Inactive

lbushi25 had a problem deploying to WindowsCILock February 8, 2024 00:28 — with GitHub Actions Error

lbushi25 added 3 commits February 7, 2024 16:52

Fix reduction tests

273fbf2

Fix reduction tests

ef2a6cc

Fix reduction tests

604893f

lbushi25 had a problem deploying to WindowsCILock February 8, 2024 00:59 — with GitHub Actions Error

lbushi25 temporarily deployed to WindowsCILock February 8, 2024 01:02 — with GitHub Actions Inactive

lbushi25 had a problem deploying to WindowsCILock February 8, 2024 02:23 — with GitHub Actions Error

Revert disable-caching to use usm

406ceb0

lbushi25 temporarily deployed to WindowsCILock February 8, 2024 03:18 — with GitHub Actions Inactive

lbushi25 had a problem deploying to WindowsCILock February 8, 2024 03:30 — with GitHub Actions Error

Minor fixes

c2ba1ad

lbushi25 temporarily deployed to WindowsCILock February 8, 2024 03:54 — with GitHub Actions Inactive

lbushi25 had a problem deploying to WindowsCILock February 8, 2024 04:04 — with GitHub Actions Failure

lbushi25 added 2 commits February 7, 2024 21:15

Add more tests

f3b2142

Add more tests

01a6431

lbushi25 temporarily deployed to WindowsCILock February 8, 2024 05:17 — with GitHub Actions Inactive

lbushi25 had a problem deploying to WindowsCILock February 8, 2024 05:29 — with GitHub Actions Failure

Fix vector initialization bug

c73e919

lbushi25 temporarily deployed to WindowsCILock February 8, 2024 06:34 — with GitHub Actions Inactive

lbushi25 had a problem deploying to WindowsCILock February 8, 2024 06:45 — with GitHub Actions Error

Fix formatting

f2bde72

Finish off rewriting regression tests

f0d325f

lbushi25 temporarily deployed to WindowsCILock February 8, 2024 20:12 — with GitHub Actions Inactive

lbushi25 changed the title ~~Rewrite tests that fail when usm_shared_allocations not supported #2~~ [SYCL] Rewrite tests that fail when usm_shared_allocations not supported #2 Feb 8, 2024

lbushi25 had a problem deploying to WindowsCILock February 8, 2024 21:06 — with GitHub Actions Error

Fix runtime error

ceff46a

lbushi25 temporarily deployed to WindowsCILock February 8, 2024 21:25 — with GitHub Actions Inactive

lbushi25 temporarily deployed to WindowsCILock February 8, 2024 22:24 — with GitHub Actions Inactive

aelovikov-intel requested a review from uditagarwal97 February 8, 2024 22:49

lbushi25 mentioned this pull request Feb 9, 2024

[SYCL] Rewrite tests when usm_ashared_allocations not supported #3 #12668

Closed

sommerlukas approved these changes Feb 9, 2024

View reviewed changes

uditagarwal97 requested changes Feb 9, 2024

View reviewed changes

Rewrite tests using malloc_device and fix casing

971e875

lbushi25 had a problem deploying to WindowsCILock February 9, 2024 23:49 — with GitHub Actions Error

Rewrite tests using malloc_device and fix casing

d7803db

lbushi25 temporarily deployed to WindowsCILock February 9, 2024 23:51 — with GitHub Actions Inactive

lbushi25 requested review from uditagarwal97 and aelovikov-intel February 9, 2024 23:52

lbushi25 had a problem deploying to WindowsCILock February 10, 2024 00:02 — with GitHub Actions Error

Fix failures

fd26740

lbushi25 temporarily deployed to WindowsCILock February 10, 2024 00:25 — with GitHub Actions Inactive

lbushi25 temporarily deployed to WindowsCILock February 10, 2024 00:36 — with GitHub Actions Inactive

cperkinsintel approved these changes Feb 12, 2024

View reviewed changes

uditagarwal97 approved these changes Feb 12, 2024

View reviewed changes

aelovikov-intel merged commit 6639e78 into intel:sycl Feb 12, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Rewrite tests that fail when usm_shared_allocations not supported #2 #12655

[SYCL] Rewrite tests that fail when usm_shared_allocations not supported #2 #12655

lbushi25 commented Feb 7, 2024 •

edited

Loading

github-actions bot commented Feb 7, 2024 •

edited

Loading

lbushi25 commented Feb 9, 2024

sommerlukas left a comment

uditagarwal97 Feb 9, 2024

lbushi25 Feb 9, 2024

uditagarwal97 Feb 9, 2024

uditagarwal97 Feb 9, 2024

uditagarwal97 Feb 9, 2024

uditagarwal97 Feb 9, 2024

lbushi25 Feb 9, 2024

uditagarwal97 Feb 9, 2024

uditagarwal97 Feb 9, 2024

uditagarwal97 Feb 9, 2024

aelovikov-intel Feb 9, 2024

uditagarwal97 Feb 9, 2024

lbushi25 Feb 9, 2024 •

edited

Loading

lbushi25 commented Feb 9, 2024

uditagarwal97 commented Feb 9, 2024

lbushi25 commented Feb 9, 2024

cperkinsintel left a comment

lbushi25 commented Feb 12, 2024

lbushi25 commented Feb 12, 2024

uditagarwal97 left a comment

[SYCL] Rewrite tests that fail when usm_shared_allocations not supported #2 #12655

[SYCL] Rewrite tests that fail when usm_shared_allocations not supported #2 #12655

Conversation

lbushi25 commented Feb 7, 2024 • edited Loading

github-actions bot commented Feb 7, 2024 • edited Loading

lbushi25 commented Feb 9, 2024

sommerlukas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbushi25 Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

lbushi25 commented Feb 9, 2024

uditagarwal97 commented Feb 9, 2024

lbushi25 commented Feb 9, 2024

cperkinsintel left a comment

Choose a reason for hiding this comment

lbushi25 commented Feb 12, 2024

lbushi25 commented Feb 12, 2024

uditagarwal97 left a comment

Choose a reason for hiding this comment

lbushi25 commented Feb 7, 2024 •

edited

Loading

github-actions bot commented Feb 7, 2024 •

edited

Loading

lbushi25 Feb 9, 2024 •

edited

Loading