-
Notifications
You must be signed in to change notification settings - Fork 51
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
Improve documentation for virtual functions #551
Conversation
- Fix typos - Fix variable names - Improve formulation for Complications and Fixes - Improve kokkos_malloc code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entry is rather confusing, not least of all because there's co-mingling of Kokkos and CUDA code. If the goal is to provide an explanation of virtual functions in Kokkos (and how to access them correctly from host and device), this entry takes the long way around the proverbial barn.
A few more suggestions:
- Point users to a couple quality C++ resources on virtual functions (you already have one)
- Remove "hybrid" code
- Remove code that is intentionally incorrect
- Focus on the correct Kokkos code (making sure the example will compile and run)
- Brief explanation of why this works (i.e., Virtual Tables, Virtual pointers)
- Maybe mention "pitfalls" to alert the reader to the fact that derived class methods on host and device, respectively, have strict access patterns, where one (host) cannot access the other (device)
I think @pzehner's goal was to update the page to make the examples work. However, I agree with @ajpowelsnl that this page is quite complicated to read, and I do not think the middle section with explicit Cuda code is very appealing to a Kokkos user. More generally, do we want to advertise using virtual functions on GPU? |
I would prefer not advising the use of virtual functions on GPUs. But I had the feeling, the page did show how it can be done while expressing the difficulties that this brings to the table in quite a neutral tone |
@ajpowelsnl As stated in the description of this PR, this contribution is an improvement of the existing documentation page. Its pedagogical approach (i.e. presenting the problem with a failing mixed CUDA-Kokkos code, then iterating to a more Kokkos solution) wasn't changed. Many remarks that you made about this PR are more remarks about the original page. My objectives were clearly stated at the beginning of this discussion. Now we're here, we can discuss about changing the page more deeply.
I don't think more resources would be necessary. If a developer even accesses this documentation page, it's reasonable to consider they know what a virtual function is. If you have any interesting reference, feel free to tell me, however.
The approach of the original article is indeed weird: nobody mixes CUDA and Kokkos code directly like that. I think it would make more sense to start from a plain serial code and get a portable(-ish) Kokkos code. Or from a full CUDA code and get a Kokkos code (which is maybe the original motivation for this page).
Naive approaches could be just described, instead of being presented in a code block.
This PR makes the Kokkos code at least buildable and proposes a full Kokkos approach.
I believe the page already explains it.
This could be improved in text, indeed. The illustrations, if I got them right, already show what is possible and not.
I think we should warn the reader at the very beginning of the article that using virtual functions in parallelized regions is a bad idea in the first place. It is known to have bad performance, it's inconvenient and ugly to use on GPU... @masterleinad confirmed me on Slack that this practice is discouraged. I'll add such a warning at the beginning of the article. |
Co-authored-by: Cédric Chevalier <cedric.chevalier019@proton.me>
According to @masterleinad, using virtual functions on device is not possible with SYCL. |
The most recent approach is explained in https://github.com/AlexeySachkov/llvm/blob/private/asachkov/virtual-functions-extension-spec/sycl/doc/design/VirtualFunctions.md which basically requires annotating virtual functions and queues. |
I profoundly modified the documentation page to remove mixed Cuda/Kokkos code and start with serial code. It'd be better to read the page in its entirety, not only the diffs that are hard to read. |
Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
Co-authored-by: Thomas Padioleau <thomas.padioleau@cea.fr>
Co-authored-by: Thomas Padioleau <thomas.padioleau@cea.fr>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only have minor nitpicking
Co-authored-by: JBludau <104908666+JBludau@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the page quite clear!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
Co-authored-by: Thomas Padioleau <thomas.padioleau@cea.fr>
Please squash merge next time |
This PR aims to deeply rework the documentation relative to virtual functions:
kokkos_malloc
code: the documentation usedKokkos::kokkos_malloc
in a way that couldn't compile. Astatic_cast
was needed. In the same Slack discussion, it was suggested to use a more precise formulation (separate allocated memory and actual object);