Repository navigation
clr(hip): ABI clean up on MSVC and GCC - #9920
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
bc7f60d to
a174c65
Compare
|
@github-copilot review |
8a56a52 to
3095c53
Compare
|
I agree with exposing a uniform "C" only interface. But, we need to make sure the APIs were not exposed in C++ for a specific reason. @mangupta can you please confirm? I believe this is also a breaking change. |
There was a problem hiding this comment.
Pull request overview
Cleans up HIP ABI exposure across MSVC and GCC builds.
Changes:
- Gives
hipError_ta fixed C++ underlying type. - Consolidates C-linkage kernel wrappers.
- Configures runtime linkage and symbol visibility.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
projects/hip/include/hip/hip_runtime_api.h |
Updates hipError_t representation. |
projects/clr/hipamd/src/hip_table_interface.cpp |
Adds C linkage to kernel APIs. |
projects/clr/hipamd/src/hip_table_interface_c.cpp |
Removes duplicate wrappers. |
projects/clr/hipamd/src/CMakeLists.txt |
Adjusts sources and visibility. |
projects/clr/CMakeLists.txt |
Modernizes MSVC runtime selection. |
Suppressed comments (1)
projects/clr/hipamd/src/hip_table_interface.cpp:2560
- This moved definition likewise has no prior
HIP_PUBLIC_APIdeclaration in this translation unit (hip_ext.h:68-76is not included). Onceamdhip64is compiled with hidden visibility, the deprecated but public C entry point cannot be exported by the version script. Give the definition default visibility or include the public declaration.
extern "C" hipError_t hipHccModuleLaunchKernel(hipFunction_t f, uint32_t globalWorkSizeX,
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
e351f61 to
36d1f91
Compare
608fd87 to
e3e62c3
Compare
129d34a to
2decb2f
Compare
warning clean up on MSVC
03df86f to
648994a
Compare
e5dfbd7 to
b86d492
Compare
b86d492 to
f9ee3b3
Compare
Previous attempt was broken and we need more work to enable this. This is because compilers under Linux are allowed to do crazy optimizations under `-fvisibility=hidden` and we need to make the compilers (not linkers) aware of public APIs.
f9ee3b3 to
b025a8d
Compare
|
Here's what I use to dump the exported symbols. Before and after this PR: |
TheRock Submodule Bump ActivityNewest first
|
Doesnt this break ABI compatibility, shouldn't the major version be updated? |
This PR cleans up a few ABI/API issues.
Motivation
Unwanted Symbols
Currently, we expose a few unwanted symbols, and this PR removes them.
hipExtModuleLaunchKernelandhipHccModuleLaunchKernel. They need to beextern "C".__gnu_h2f_ieeeand__gnu_f2h_ieee. These are from libgcc. We should not be providing these symbols.MSVC Warning
Right now MSVC generates many warnings because of a cast from
hipError_ttouint32_t. This PR uses C++ enum classes to fix this warning. This explicitly specifies thathipError_tshould be auint32_t, which makes the ABI clearer (under Windows).Hidden Visibility by Default.
Under Linux, all symbols are visible by default. While this is the default behaviour, it can cause unwanted conflicts during loading (because ELF uses a flat namespace). This PR uses
-fvisibililty=hiddento hide symbols by default.To expose public APIs, we define
HIP_PUBLIC_APIas__attribute__((visibility=default))so that the compiler can expose this during compilation phase. Under GCC, this is essential to prevent compiler from optimizing the function into internal linkage.We also need to remove
HIP_PUBLIC_APIfrom the public header, since now this contains symbol visibility attribute and we do not want to force applications to export our symbols. This macro is moved tohip_internal.hppand it's exclusively designed for HIP developers now: if you would like to implement a public API (inhip_runtime_api.h), includehip_internal.hppand decorate your function withHIP_PUBLIC_API.Issue Tracking
JIRAID : AIRUNTIME-2633 Close
Test Result
All tests link and pass fine. We shouldn't use the private ABI for tests.
Submission Checklist