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] Add missing symbols hip #1031

Closed
wants to merge 2 commits into from

Conversation

hdelan
Copy link
Contributor

@hdelan hdelan commented Nov 2, 2023

The global exported symbols are here https://github.com/oneapi-src/unified-runtime/blob/adapters/source/adapters/adapter.map.in

This file https://github.com/oneapi-src/unified-runtime/blob/adapters/source/adapters/hip/ur_interface_loader.cpp is missing some symbols that are written in the lib map. This caused a linker error for LLD when linking in DPC++. This patch adds them, fixing the linker error.

@hdelan hdelan requested a review from a team as a code owner November 2, 2023 12:51
@hdelan hdelan closed this Nov 3, 2023
@hdelan hdelan reopened this Nov 3, 2023
@kbenzie
Copy link
Contributor

kbenzie commented Nov 3, 2023

The testing doesn't like this change, will need to investigagte that before we can merge.

@hdelan
Copy link
Contributor Author

hdelan commented Nov 3, 2023

The testing doesn't like this change, will need to investigagte that before we can merge.

Investigating

@pbalcer
Copy link
Contributor

pbalcer commented Nov 3, 2023

Looking at the loader implementation, it expects the addr table to at least be null-initialized.

@pbalcer
Copy link
Contributor

pbalcer commented Nov 3, 2023

It also just occurred to me that you can't return an error from these functions if you want the adapter to be functional. Once one get table function fails, the rest of them are skipped (https://github.com/oneapi-src/unified-runtime/blob/main/source/loader/ur_ldrddi.cpp#L8626).

I think we have a bug that such platform is then made accessible to the user.

@hdelan
Copy link
Contributor Author

hdelan commented Nov 3, 2023

It also just occurred to me that you can't return an error from these functions if you want the adapter to be functional. Once one get table function fails, the rest of them are skipped (https://github.com/oneapi-src/unified-runtime/blob/main/source/loader/ur_ldrddi.cpp#L8626).

I think we have a bug that such platform is then made accessible to the user.

Yeah the testing passes if you change ret to UR_RESULT_SUCCESS. I'm not sure if this is desirable but have added a comment in the code to say we need this to return UR_RESULT_SUCCESS or else platform initialisation will fail.

@hdelan
Copy link
Contributor Author

hdelan commented Nov 14, 2023

@kbenzie any word on this? Would be nice to fix lld builds in DPC++

Copy link
Contributor

@mmoadeli mmoadeli left a comment

Choose a reason for hiding this comment

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

Is there any need for a (warning or error ) message inside the functions annotated with // TODO: Implement ?

@kbenzie
Copy link
Contributor

kbenzie commented Nov 14, 2023

@kbenzie any word on this? Would be nice to fix lld builds in DPC++

We need to prioritise merging PR's for the next release which this isn't targetted for. Will aim to merge this after we've drained that release PR backlog.

@hdelan
Copy link
Contributor Author

hdelan commented Nov 14, 2023

Is there any need for a (warning or error ) message inside the functions annotated with // TODO: Implement ?

We need these entry points to pass or else the loader initialisation will fail, so it can't error out. I'm not sure about issuing a warning. Perhaps this is something that could be done once the UR_ADAPTER_SPECIFIC_ERROR is split into a separate warning and error val. See here #762 (comment)

@hdelan hdelan added the ready to merge Added to PR's which are ready to merge label Nov 15, 2023
@hdelan hdelan mentioned this pull request Nov 21, 2023
@hdelan hdelan closed this Dec 4, 2023
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.

4 participants