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

Document GPU attributes #25884

Merged
merged 9 commits into from
Sep 5, 2024
Merged

Conversation

DanilaFe
Copy link
Contributor

@DanilaFe DanilaFe commented Sep 5, 2024

Closes #23149.

This PR implements my initial vision for documenting attributes based on procedures that implement them. In PR #25826, I made GPU attributes be backed by Chapel functions. One of my goals with this approach was to be able to document these attributes using the existing Chapel mechanism (doc comments attached to procedures). This PR does so, by adding a new chpldoc attribute (@chpldoc.attributeSignature) that marks a function as defining the signature (formals etc.) of an attribute.

Since the functions in the GPU module that back attributes are internal (not intended to be called directly), they would normally be skipped by chpldoc's output. This PR adjusts the behavior of chpldoc to not skip the functions in this case.

It also adds documentation to the various attributes using this new mechanism.

This PR depends on chapel-lang/sphinxcontrib-chapeldomain#97, which implements the RST-to-HTML conversion of the new 'annotation' directive chpldoc will emit.

Screen Shot 2024-09-05 at 10 16 43 AM

Reviewed by @e-kayrakli, @mppf, and @lydia-duncan -- thanks!

Testing

  • paratest (just in case)

Copy link
Member

@mppf mppf left a comment

Choose a reason for hiding this comment

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

The chpldoc changes look good to me. Please ask somebody more familiar with the GPU effort to review the new GPU documentation itself.

Copy link
Contributor

@e-kayrakli e-kayrakli left a comment

Choose a reason for hiding this comment

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

Doc-wise, I strongly recommend not focusing on variable declarations as that feels irrelevant from the user's standpoint. I also have some other minor feedback in-line.

is likely an issue, so the warning is emitted by default. However, in
case the user is aware of this and wants to silence the warning, they can
set this configuration parameter to ``true``.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR, but can we use this param in lieu of CHPL_GPU_NO_CPU_MODE_WARNING in cpu-as-device? Notionally, they serve the same purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I seem to recall config params being used for this purpose, at least during the 2.0 stabilization push, but an environment variable is fine with me as well.

(shakes fist at copilot)

Signed-off-by: Danila Fedorin <[email protected]>
@DanilaFe DanilaFe marked this pull request as ready for review September 5, 2024 20:43
@DanilaFe DanilaFe merged commit 20de6bb into chapel-lang:main Sep 5, 2024
7 checks passed
DanilaFe added a commit that referenced this pull request Sep 18, 2024
This documents and makes use of the various changes I made in
#25826,
chapel-lang/sphinxcontrib-chapeldomain#97, and
#25884.

Reviewed by @e-kayrakli -- thanks!
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.

Should we provide a specific way to document attributes?
4 participants