Skip to content

Conversation

@randyh62
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Continuous Integration

What were the changes?

Combined content from HIP porting guide and HIP driver porting guide.
Updated text as needed.

Why are these changes needed?

The two topics covered similar concepts, and needed to be updated.

Updated CHANGELOG?

  • Yes
  • No, Does not apply to this PR.

Added/Updated documentation?

  • Yes
  • No, Does not apply to this PR.

Additional Checks

  • I have added tests relevant to the introduced functionality, and the unit tests are passing locally.
  • Any dependent changes have been merged.

@randyh62 randyh62 self-assigned this Sep 13, 2025
@randyh62 randyh62 added ci:docs-only Only run Read the Docs CI on this PR documentation labels Sep 13, 2025
Copy link

@j-stephan j-stephan left a comment

Choose a reason for hiding this comment

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

Good work, I think this is a really nice unification of both pages. Some remarks:

called. When ``__hipRegisterFunction`` is called, the stub functions are
associated with the corresponding kernels in the code objects.

HIP-Clang implements two sets of APIs for launching kernels.

Choose a reason for hiding this comment

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

What is the second set?

Copy link
Contributor Author

@randyh62 randyh62 Sep 18, 2025

Choose a reason for hiding this comment

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

HIP-Clang implements two sets of APIs for launching kernels:

  1. By default, when HIP-Clang encounters the <<<>>> statement in the host code, it first calls hipConfigureCall to set up the threads and grids. It then calls the stub function with the given arguments.
  2. The stub function calls hipSetupArgument for each kernel argument, then calls hipLaunchByPtr with a function pointer to the stub function. In hipLaunchByPtr, the actual kernel associated with the stub function is launched.

-or-

  1. By default, when HIP-Clang encounters the <<<>>> statement in the host code, it first calls hipConfigureCall to set up the threads and grids. It then calls the stub function with the given arguments. The stub function calls hipSetupArgument for each kernel argument, then calls hipLaunchByPtr with a function pointer to the stub function.
  2. In hipLaunchByPtr, the actual kernel associated with the stub function is launched.

Choose a reason for hiding this comment

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

Are those distinct sets? When reading the paragraph from top to bottom, this sounds like a logical sequence of events:

  • Encounter <<<>>>, then
  • call hipConfigureCall for the grid configuration, then
  • call the stub function, then
  • call hipSetupArgument for each kernel argument, then
  • call hipLaunchByPtr, then
  • launch actual kernel.

I think we need some input from the HIP-Clang team here, I assume the "two sets" description comes from them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jujiang-del or @yxsamliu can you comment on this question and the issue of the two sets of APIs for launching kernels?

@randyh62 randyh62 requested a review from j-stephan September 19, 2025 22:03
Copy link

@j-stephan j-stephan left a comment

Choose a reason for hiding this comment

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

Looks good overall. A few small things I've noticed:

@neon60
Copy link
Contributor

neon60 commented Sep 24, 2025

Please add these changes too:
randyh62@487299f

@neon60
Copy link
Contributor

neon60 commented Sep 30, 2025

@randyh62 I do not have more comments. Others looks good.

Updated from additional comments from Jan
fix :cpp:type:`` errors
@randyh62 randyh62 requested a review from yxsamliu October 1, 2025 20:22
Copy link

@lpaoletti lpaoletti left a comment

Choose a reason for hiding this comment

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

I'm done

@emankov
Copy link
Contributor

emankov commented Oct 3, 2025

@randyh62, just a minor suggestion about PRs like this. Please separate PRs that change and move code. It would drastically save reviewing time. Thanks!

Implement Leo's feedback.
Added Istvan and Evgeniy comments
@randyh62 randyh62 merged commit ec50ff0 into ROCm:docs/develop Oct 14, 2025
1 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:docs-only Only run Read the Docs CI on this PR documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants