Skip to content

[SYCL] Fix ProgramManager::kernelUsesAssert to avoid tmp string creation #17912

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 9, 2025

Conversation

vinser52
Copy link
Contributor

@vinser52 vinser52 commented Apr 8, 2025

The handler calls the ProgramManager::kernelUsesAssert with const char * argument. It resulted in a temporary std::string creation. This PR changes the ProgramManager::kernelUsesAssert to accept template parameter and uses heterogeneous lookup feature of std::set

@vinser52 vinser52 requested a review from a team as a code owner April 8, 2025 16:50
@vinser52 vinser52 requested a review from steffenlarsen April 8, 2025 16:50
@vinser52 vinser52 changed the title Fix ProgramManager::kernelUsesAssert to avoid tmp string creation [SYCL] Fix ProgramManager::kernelUsesAssert to avoid tmp string creation Apr 8, 2025
@vinser52 vinser52 requested a review from sergey-semenov April 8, 2025 16:50
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.

Neat! You learn something new every day. 🚀

// different types without temporary key_type object creation. This includes
// standard overloads, such as comparison between std::string and
// std::string_view or just char*.
using KernelUsesAssertSet = std::set<std::string, std::less<>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit; This alias feels a little overkill when it's just used here and in a unittest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it is a common practice in C++ to use aliases for container types. As you pointed out, there are already two places. If we decide to change the container type in the future it is easier with alias.

@vinser52
Copy link
Contributor Author

vinser52 commented Apr 9, 2025

@steffenlarsen should we merge this PR?

@sergey-semenov sergey-semenov merged commit 8955813 into intel:sycl Apr 9, 2025
24 checks passed
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