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.

Sign up for GitHub

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

Jump to bottom

Refactor C++ language extensions and C++ support #3673

Open
wants to merge 2 commits into
base: docs/develop
Choose a base branch
from
Open

Refactor C++ language extensions and C++ support #3673

wants to merge 2 commits into from

Conversation

MKKnorr
Copy link

@MKKnorr MKKnorr commented Nov 15, 2024

No description provided.

@MKKnorr MKKnorr force-pushed the refactor-cpp-language-extensions branch 2 times, most recently from e911a63 to da93aea Compare November 15, 2024 14:34
@neon60 neon60 deleted the branch docs/develop November 18, 2024 17:39
@neon60 neon60 closed this Nov 18, 2024
@neon60 neon60 reopened this Nov 18, 2024
@MKKnorr MKKnorr changed the base branch from hip_runtime_api_how-to to docs/develop November 19, 2024 09:34
@MKKnorr MKKnorr force-pushed the refactor-cpp-language-extensions branch 2 times, most recently from 760259e to 6d6c430 Compare November 21, 2024 17:01
@MKKnorr MKKnorr marked this pull request as ready for review November 21, 2024 17:01
@MKKnorr MKKnorr requested a review from neon60 November 21, 2024 17:02
@MKKnorr MKKnorr force-pushed the refactor-cpp-language-extensions branch 3 times, most recently from 058fdde to 3eddfa4 Compare November 22, 2024 10:43
@MKKnorr MKKnorr changed the base branch from docs/develop to landing_page_2 November 22, 2024 10:43
@MKKnorr MKKnorr force-pushed the refactor-cpp-language-extensions branch from 3eddfa4 to c485169 Compare November 22, 2024 10:45
@neon60
Copy link
Contributor

neon60 commented Nov 24, 2024

@MKKnorr Please rebase on docs/develop.

@MKKnorr MKKnorr changed the base branch from landing_page_2 to docs/develop November 25, 2024 09:42
@MKKnorr MKKnorr force-pushed the refactor-cpp-language-extensions branch 2 times, most recently from ac2b239 to 761c1f8 Compare November 25, 2024 09:48
@MKKnorr MKKnorr force-pushed the refactor-cpp-language-extensions branch 2 times, most recently from 58960c9 to 4b37cf5 Compare November 25, 2024 15:10
@MKKnorr MKKnorr force-pushed the refactor-cpp-language-extensions branch from 4b37cf5 to 0cf8414 Compare November 26, 2024 13:05
randyh62
randyh62 approved these changes Nov 27, 2024
View reviewed changes
Copy link

@randyh62 randyh62 left a comment

Choose a reason for hiding this comment

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

Looks ok to me. I left a few comments.

docs/how-to/hip_cpp_language_extensions.rst Outdated Show resolved Hide resolved
docs/how-to/hip_cpp_language_extensions.rst Show resolved Hide resolved
docs/how-to/hip_cpp_language_extensions.rst Outdated Show resolved Hide resolved
docs/how-to/hip_cpp_language_extensions.rst Outdated Show resolved Hide resolved
docs/how-to/hip_cpp_language_extensions.rst
automatically reduce shared memory usage. The compilation fails, if the compiler
can not generate code that satisfies the launch bounds.

On NVCC this parameter maps to the ``.maxntid`` PTX directive.
Copy link

Choose a reason for hiding this comment

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

Suggested change
On NVCC this parameter maps to the ``.maxntid`` PTX directive.
On NVCC this parameter maps to the ``.maxntid`` PTX directive.

Is this needed?

Copy link
Author

Choose a reason for hiding this comment

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

Yes and no. This and the "porting from CUDA __launch_bounds" section should go to the porting guide, but that is being done in another issue. I'll add a note to that issue

docs/how-to/hip_cpp_language_extensions.rst
HIP supports ``__threadfence()``, ``__threadfence_block()`` and
``__threadfence_system()``.

On AMD devices, ``__threadfence_system()``, has restrictions and therefore needs
Copy link

Choose a reason for hiding this comment

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

This feels like it is referring to some implicit knowledge the user must have relative to threadfence_system? What are the "restrictions"? And is it appropriate to call it a workaround in the docs? Seems like a workaround is good for an issue or bug report, but not for user docs. Perhaps "requires the following process"?

Copy link
Author

Choose a reason for hiding this comment

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

I wish I had more information on this, but I couldn't find information proving or disproving this, so I just reformatted the already existing content

docs/how-to/kernel_language_cpp_support.rst
kernel launch is present in the code.

To compile device code and include kernel launches, a compiler with full HIP
support is needed. On AMD platforms this is ``amdclang++``, whereas on NVIDIA
Copy link

Choose a reason for hiding this comment

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

I would remove the reference to NVIDIA and nvcc

Copy link
Author

Choose a reason for hiding this comment

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

Then I also wouldn't mention the other ones. It is kind of weird to say that "On AMD platforms this is amdclang, but you can also use hipcc which is a wrapper around it"

docs/how-to/kernel_language_cpp_support.rst Outdated Show resolved Hide resolved
docs/how-to/kernel_language_cpp_support.rst
An important difference between the host and device code C++ support is
exception handling. In device code, exceptions aren't available due to
the hardware architecture. The device code must use return codes to handle
errors.
Copy link

Choose a reason for hiding this comment

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

Can we add an example?

Copy link
Author

Choose a reason for hiding this comment

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

I think an example here would be overkill. Also it is quite hard to show an example of what you can not do

docs/how-to/kernel_language_cpp_support.rst
Kernel Compilation
================================================================================

``hipcc`` now supports compiling C++/HIP kernels to binary code objects. The
Copy link

Choose a reason for hiding this comment

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

Suggested change
``hipcc`` now supports compiling C++/HIP kernels to binary code objects. The
``hipcc`` now supports compiling C++/HIP kernels to binary code objects. The

Can we replace hipcc here with amdclang++?

Copy link
Author

Choose a reason for hiding this comment

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

Nope. As an aside, I think this whole section about "Kernel compilation" does not belong here, but there is also a separate issue about that

@MKKnorr MKKnorr force-pushed the refactor-cpp-language-extensions branch from ef46f97 to 00a1e34 Compare November 28, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@randyh62 randyh62 randyh62 approved these changes

@neon60 neon60 neon60 approved these changes

@chrispaquot chrispaquot Awaiting requested review from chrispaquot chrispaquot is a code owner

@gandryey gandryey Awaiting requested review from gandryey gandryey is a code owner

@saleelk saleelk Awaiting requested review from saleelk saleelk is a code owner

@mangupta mangupta Awaiting requested review from mangupta mangupta is a code owner

@rakesroy rakesroy Awaiting requested review from rakesroy rakesroy is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MKKnorr @neon60 @randyh62