Skip to content

Conversation

@jcosborn
Copy link
Contributor

@jcosborn jcosborn commented Jan 4, 2025

This will allow the SYCL target to allocate shared memory at kernel launch and pass the pointer in through the KernelOps structure.

@jcosborn jcosborn requested a review from a team as a code owner January 4, 2025 23:00
Copy link
Member

@maddyscientist maddyscientist left a comment

Choose a reason for hiding this comment

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

I've left some comments on the PR, but the main issue I have is with
1.) The lack of documentation regarding using KernelOps. E.g., how does someone who is writing a new kernel that uses shared memory know what to do. We need a extensive wiki page describing this.
2.) How KernelOps will be tested in CI without any SYCL target. How can we ensure that KernelOps are developed correctly?

We can discuss in our call.

@jcosborn
Copy link
Contributor Author

I've left some comments on the PR, but the main issue I have is with 1.) The lack of documentation regarding using KernelOps. E.g., how does someone who is writing a new kernel that uses shared memory know what to do. We need a extensive wiki page describing this. 2.) How KernelOps will be tested in CI without any SYCL target. How can we ensure that KernelOps are developed correctly?

We can discuss in our call.

I've added some documentation here
https://github.com/lattice/quda/wiki/KernelOps-usage
It will need to be updated as more pieces are merged in from the SYCL branch.
Let me know if anything isn't clear so far.

As we discussed on the call, the KernelOps handling should be fairly robust (once fully merged), so that instantiating tracked operations will require passing in a kernel functor that inherited from the appropriate KernelOps type. This type checking happens at compile time.

@maddyscientist
Copy link
Member

maddyscientist commented Jan 16, 2025

I've added some documentation here https://github.com/lattice/quda/wiki/KernelOps-usage
It will need to be updated as more pieces are merged in from the SYCL branch. Let me know if anything isn't clear so far.

Documentation is exactly what I wanted, thanks @jcosborn.

@weinbe2
Copy link
Contributor

weinbe2 commented Jan 30, 2025

This design looks good to me given the requirements, thanks @jcosborn. The wiki page is greatly appreciated.

I don't "like" the decltype(*this) that needed to be used in one of the coarse-related file, but since it's a one-off in a single file I'll let it go. I understand why template deduction couldn't be used without a bunch of unnecessary function argument rearranging.

My only request is for the wiki page; I think the links to files + line-numbers should be modified to be permalinks to a specific commit as opposed to just develop. This isn't a blocking requirement for this PR; if anything it can be updated with the commit id of the merge.

@jcosborn
Copy link
Contributor Author

Thanks for the review. Making the wiki use permalinks is a good idea. Once this is merged I'll update it with permalinks to develop.

@jcosborn
Copy link
Contributor Author

I've fixed the CI, and I think everything has been addressed (except for updating the docs after the merge).
Let me know if there is anything else.

Copy link
Member

@maddyscientist maddyscientist left a comment

Choose a reason for hiding this comment

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

Loos good now. Thanks for the work on this @jcosborn.

@maddyscientist
Copy link
Member

@jcosborn can you merge in the latest version of develop? This will fix the CSCS pipeline which would be good to run before we merge this in.

@weinbe2
Copy link
Contributor

weinbe2 commented Mar 19, 2025

@jcosborn merge in develop one last time to get the csci running, and assuming there are no issues I'll hit merge tomorrow, I promise!

@weinbe2 weinbe2 merged commit f2e065e into develop Mar 19, 2025
7 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