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

[HIP] Remove inline from helper #1178

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

hdelan
Copy link
Contributor

@hdelan hdelan commented Dec 12, 2023

Different compilers can change the linkage of symbols based on whether inline is used. This removes the inline specifier so it has external linkage and can be called from within another TU.

@hdelan hdelan requested a review from a team as a code owner December 12, 2023 14:10
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (69a56ea) 15.72% compared to head (526f7e6) 15.72%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1178      +/-   ##
==========================================
- Coverage   15.72%   15.72%   -0.01%     
==========================================
  Files         223      223              
  Lines       31484    31484              
  Branches     3556     3556              
==========================================
- Hits         4952     4951       -1     
- Misses      26481    26482       +1     
  Partials       51       51              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@ldrumm ldrumm left a comment

Choose a reason for hiding this comment

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

Now this function is visible, does it need to start with ur or some internal-private prefix?

@kbenzie will know

@kbenzie
Copy link
Contributor

kbenzie commented Dec 12, 2023

Now this function is visible, does it need to start with ur or some internal-private prefix?

@kbenzie will know

If this is only being made to have external linkage for use within the HIP adapter there is no need for it to have a ur preifx. We have linker scripts for stripping symbols which we don't want to be part of the official ABI of the adapters.

@hdelan
Copy link
Contributor Author

hdelan commented Dec 12, 2023

Now this function is visible, does it need to start with ur or some internal-private prefix?

@kbenzie will know

This function already had external linkage with the compilers that I tested (gcc 11 and upstream clang) so it shouldn't need any change. Note that this only broke for gcc 12 since the func is called from another TU and gcc 12 seems to have some implementation defined behaviour that makes inline funcs have internal linkage

@ldrumm
Copy link
Contributor

ldrumm commented Dec 12, 2023

Thanks for clarifying

@hdelan hdelan added the ready to merge Added to PR's which are ready to merge label Jan 5, 2024
@hdelan
Copy link
Contributor Author

hdelan commented Jan 8, 2024

Ping @oneapi-src/owner-unified-runtime can we merge this?

@kbenzie
Copy link
Contributor

kbenzie commented Jan 8, 2024

Ping @oneapi-src/owner-unified-runtime can we merge this?

We are working though the ready to merge backlog and will get to this as soon as we can.

@hdelan
Copy link
Contributor Author

hdelan commented Jan 8, 2024

Thanks @kbenzie

@aarongreig aarongreig merged commit 25e0b60 into oneapi-src:main Jan 12, 2024
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Added to PR's which are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants