Skip to content
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

[SYCL] Write tests following the test plan for the work group memory extension #15928

Merged
merged 37 commits into from
Nov 19, 2024

Conversation

lbushi25
Copy link
Contributor

@lbushi25 lbushi25 commented Oct 30, 2024

This PR adds feature tests as per the work group memory extension test plan.

@lbushi25 lbushi25 marked this pull request as ready for review October 30, 2024 17:11
@lbushi25 lbushi25 requested a review from a team as a code owner October 30, 2024 17:11
@lbushi25 lbushi25 requested review from a team as code owners October 31, 2024 16:27
}
}

// Explicit instantiations for the relevant data types.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why are these explicit instantiations necessary?

Copy link
Contributor Author

@lbushi25 lbushi25 Oct 31, 2024

Choose a reason for hiding this comment

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

For some reason, when passing these kernel names to the free function queries with concrete template types, it would complain that these names do not represent kernels because they had not been instantiated yet although one would hope this instantiation would be automatic upon passing these kernel names to the queries.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds like either a bug or a missing feature with free function kernels. Can you add the same comment here about removing this code when free function kernels are fully supported?

Alternatively, maybe we should just delay testing of work_group_memory with free function kernels until there is better support for free function kernels. It seems like there are a lot of places where you need to add workarounds for missing features in free function kernels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds like either a bug or a missing feature with free function kernels. Can you add the same comment here about removing this code when free function kernels are fully supported?

Alternatively, maybe we should just delay testing of work_group_memory with free function kernels until there is better support for free function kernels. It seems like there are a lot of places where you need to add workarounds for missing features in free function kernels.

Sure. We can keep the test as I've put an XFAIL directive to expect failures on all platforms since support is not implemented yet.

@lbushi25 lbushi25 changed the title [SYCL] Add indeterminate constructor to work group memory class and write tests following the test plan for the extension [SYCL] Write tests following the test plan for the work group memory extension Nov 6, 2024
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.

LGTM!

@AlexeySachkov AlexeySachkov merged commit f741bdd into intel:sycl Nov 19, 2024
13 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.

4 participants