[SYCL] Add basic tests for virtual functions #14209

AlexeySachkov · 2024-06-18T09:33:38Z

Test plan is available in #10540

…ual-functions-basic-e2e-tests

steffenlarsen

Looks good overall. One smallish question.

steffenlarsen · 2024-07-09T10:32:31Z

sycl/test-e2e/VirtualFunctions/2/1/1/missing-overrides.cpp

+                sizeof(SubstractBy4), sizeof(SubstractBy4AndMultiplyBy4)});
+
+  sycl::buffer<char> ObjStorage(sycl::range{Size});
+  char HostStorage[Size];


Do we need to set the alignment of these to correspond to some common alignment between the types we may place in it?

Good question, I think we should indeed align the storage as well. basic.align.1 says:

Attempting to create an object ([intro.object]) in storage that does not meet the alignment requirements of the object's type is undefined behavior.

Resolved in 04dda01

Main motivation is to make sure that storage for objects is properly aligned so there are no UBs when using placement new. It also allowed to simplify tests source code by outlining some common/similar object construction part into a helper.

…ual-functions-basic-e2e-tests

steffenlarsen

Looks good!

steffenlarsen

🚀

AlexeySachkov · 2024-07-18T07:10:51Z

Failure in host task does not seem related, or I don't understand how adding new tests could lead to timeout on the old one (and that was per-test timeout, not per-suite). I've reported the failure in #14613 and will proceed with the merge

aelovikov-intel · 2024-08-14T19:10:20Z

sycl/test-e2e/VirtualFunctions/helpers.hpp

+      return nullptr;
+    }
+
+    return constructHelper<RetT, T...>(TypeIndex, 0);


I think something like

llvm/sycl/include/sycl/vector.hpp

Lines 1204 to 1209 in 8eeb60d

static constexpr auto get_vec_idx(int idx) {

int counter = 0;

int result = -1;

((result = counter++ == idx ? Indexes : result), ...);

return result;

}

can be used to eliminate the helper.

TypeIndex is a runtime value here, the function is not constexpr.
Otherwise, I think I could have used tuple_element as built-in helper, essentially

UPD: looking at it more, TypeIndex being RT value shouldn't matter here. Checks are happening at runtime, only expansion happens at compile-time and list of types is known. I will take a deeper look to see if I can simplify this

[SYCL] Add basic tests for virtual functions

1d9bf7a

AlexeySachkov temporarily deployed to WindowsCILock June 18, 2024 09:34 — with GitHub Actions Inactive

AlexeySachkov temporarily deployed to WindowsCILock June 18, 2024 09:47 — with GitHub Actions Inactive

Switch to using core.hpp instead of sycl.hpp

b6d653b

AlexeySachkov had a problem deploying to WindowsCILock June 20, 2024 13:15 — with GitHub Actions Failure

AlexeySachkov had a problem deploying to WindowsCILock June 20, 2024 13:30 — with GitHub Actions Error

Apply clang-format

430a8fe

AlexeySachkov temporarily deployed to WindowsCILock June 20, 2024 13:52 — with GitHub Actions Inactive

AlexeySachkov had a problem deploying to WindowsCILock June 20, 2024 14:12 — with GitHub Actions Error

One more attempt to use the right header in E2E tests

4e29ba5

AlexeySachkov had a problem deploying to WindowsCILock June 20, 2024 15:15 — with GitHub Actions Failure

AlexeySachkov temporarily deployed to WindowsCILock June 20, 2024 16:36 — with GitHub Actions Inactive

AlexeySachkov had a problem deploying to WindowsCILock June 21, 2024 08:31 — with GitHub Actions Failure

AlexeySachkov had a problem deploying to WindowsCILock June 21, 2024 08:44 — with GitHub Actions Failure

Merge remote-tracking branch 'origin/sycl' into private/asachkov/virt…

b7e032d

…ual-functions-basic-e2e-tests

AlexeySachkov temporarily deployed to WindowsCILock July 2, 2024 12:12 — with GitHub Actions Inactive

AlexeySachkov had a problem deploying to WindowsCILock July 2, 2024 12:40 — with GitHub Actions Failure

Merge remote-tracking branch 'origin/sycl' into private/asachkov/virt…

d9a7235

…ual-functions-basic-e2e-tests

AlexeySachkov temporarily deployed to WindowsCILock July 3, 2024 09:34 — with GitHub Actions Inactive

AlexeySachkov temporarily deployed to WindowsCILock July 3, 2024 09:48 — with GitHub Actions Inactive

AlexeySachkov marked this pull request as ready for review July 3, 2024 18:41

AlexeySachkov requested a review from a team as a code owner July 3, 2024 18:41

AlexeySachkov requested a review from steffenlarsen July 3, 2024 18:41

steffenlarsen approved these changes Jul 9, 2024

View reviewed changes

AlexeySachkov added 2 commits July 12, 2024 09:56

Introduce more helper structures

04dda01

Main motivation is to make sure that storage for objects is properly aligned so there are no UBs when using placement new. It also allowed to simplify tests source code by outlining some common/similar object construction part into a helper.

Merge remote-tracking branch 'origin/sycl' into private/asachkov/virt…

a5cdf6e

…ual-functions-basic-e2e-tests

AlexeySachkov requested a review from steffenlarsen July 15, 2024 08:02

AlexeySachkov temporarily deployed to WindowsCILock July 15, 2024 08:03 — with GitHub Actions Inactive

AlexeySachkov had a problem deploying to WindowsCILock July 15, 2024 08:27 — with GitHub Actions Failure

steffenlarsen approved these changes Jul 16, 2024

View reviewed changes

Fix issues on windows and improve error reporting

73d31b9

AlexeySachkov temporarily deployed to WindowsCILock July 17, 2024 12:07 — with GitHub Actions Inactive

steffenlarsen approved these changes Jul 17, 2024

View reviewed changes

AlexeySachkov temporarily deployed to WindowsCILock July 17, 2024 13:26 — with GitHub Actions Inactive

AlexeySachkov mentioned this pull request Jul 18, 2024

HostInteropTask/host-task-failure.cpp timed out on an unrelated change #14613

Open

AlexeySachkov merged commit d814b4a into intel:sycl Jul 18, 2024
13 of 14 checks passed

aelovikov-intel reviewed Aug 14, 2024

View reviewed changes

AlexeySachkov mentioned this pull request Sep 16, 2024

[SYCL][E2E] Add more tests for virtual functions #15067

Merged

AlexeySachkov deleted the private/asachkov/virtual-functions-basic-e2e-tests branch October 9, 2024 08:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYCL] Add basic tests for virtual functions #14209

[SYCL] Add basic tests for virtual functions #14209

Uh oh!

AlexeySachkov commented Jun 18, 2024

Uh oh!

steffenlarsen left a comment

Uh oh!

steffenlarsen Jul 9, 2024

Uh oh!

AlexeySachkov Jul 10, 2024

Uh oh!

AlexeySachkov Jul 15, 2024

Uh oh!

steffenlarsen left a comment

Uh oh!

steffenlarsen left a comment

Uh oh!

AlexeySachkov commented Jul 18, 2024

Uh oh!

Uh oh!

aelovikov-intel Aug 14, 2024

Uh oh!

AlexeySachkov Sep 16, 2024 •

edited

Loading

Uh oh!

Uh oh!

	static constexpr auto get_vec_idx(int idx) {
	int counter = 0;
	int result = -1;
	((result = counter++ == idx ? Indexes : result), ...);
	return result;
	}

[SYCL] Add basic tests for virtual functions #14209

[SYCL] Add basic tests for virtual functions #14209

Uh oh!

Conversation

AlexeySachkov commented Jun 18, 2024

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

steffenlarsen Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov commented Jul 18, 2024

Uh oh!

Uh oh!

aelovikov-intel Aug 14, 2024

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov Sep 16, 2024 • 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!

Uh oh!

AlexeySachkov Sep 16, 2024 •

edited

Loading