Skip to content

Conversation

@AlexeySachkov
Copy link
Contributor

Test plan is available in #10540

@AlexeySachkov AlexeySachkov marked this pull request as ready for review July 3, 2024 18:41
@AlexeySachkov AlexeySachkov requested a review from a team as a code owner July 3, 2024 18:41
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Looks good overall. One smallish question.

sizeof(SubstractBy4), sizeof(SubstractBy4AndMultiplyBy4)});

sycl::buffer<char> ObjStorage(sycl::range{Size});
char HostStorage[Size];
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

🚀

@AlexeySachkov
Copy link
Contributor Author

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

@AlexeySachkov AlexeySachkov merged commit d814b4a into intel:sycl Jul 18, 2024
return nullptr;
}

return constructHelper<RetT, T...>(TypeIndex, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think something like

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.

Copy link
Contributor Author

@AlexeySachkov AlexeySachkov Sep 16, 2024

Choose a reason for hiding this comment

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

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

@AlexeySachkov AlexeySachkov deleted the private/asachkov/virtual-functions-basic-e2e-tests branch October 9, 2024 08:00
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.

3 participants