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

Add HIPRTC, glorious ersatz for NVRTC #1097

Merged
merged 20 commits into from
May 16, 2019
Merged

Add HIPRTC, glorious ersatz for NVRTC #1097

merged 20 commits into from
May 16, 2019

Conversation

AlexVlx
Copy link
Contributor

@AlexVlx AlexVlx commented May 10, 2019

No description provided.

iotamudelta
iotamudelta previously approved these changes May 10, 2019
Copy link
Contributor

@iotamudelta iotamudelta left a comment

Choose a reason for hiding this comment

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

Truly glorious - thank you very much! I have verified this work with PT.

Copy link
Contributor

@mangupta mangupta left a comment

Choose a reason for hiding this comment

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

PR #1077 has been merged. This PR needs to be reworked to resolve conflicts.

iotamudelta
iotamudelta previously approved these changes May 14, 2019
Copy link
Contributor

@iotamudelta iotamudelta left a comment

Choose a reason for hiding this comment

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

Works after the changes to accommodate #1077

Change-Id: Ie499e92dfe4e5c94634b1c2b76cf52d241bcfea3
if (p->compiled) return HIPRTC_ERROR_COMPILATION;

static const string hipcc{
getenv("HIP_PATH") ? (getenv("HIP_PATH") + string{"/bin/hipcc"})
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably look for hipcc relative to libhiprtc.so. This is because HIP unit tests don't set env var HIP_PATH and we usually don't install to /opt/rocm when building unit tests. So these tests will fail to find the right hipcc.
Maybe we can do something like (sanity checks/error handling not included):

#include <dlfcn.h>
Dl_info dl_info;
dladdr((void*)hiprtcCompileProgram, &dl_info);
string path(dl_info.dli_fname); // path = /path/to/hip/lib/libhiprtc.so
path = path.substr(0, path.find_last_of("/")).substr(0, path.find_last_of("/")); // path = /path/to/hip
static const string hipcc{ path + string{"/bin/hipcc"}};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am against this. HIPRTC should just work with any hipcc, there is no such thing as the right hipcc. Moreover, the user could decide to interpose a flavour of libhiprtc.so that lives wherever he / she chooses, and then bad things happen. The test itself should be changed to set HIP_PATH, this alternative is more like the tail wagging the dog.

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically you are right. However here is the issue. HIP runtime has a close relationship with the HCC version. To ensure we are using the right HCC, hipcc has a strict check to confirm that the HCC version that was used to build HIP runtime and the HCC version that is currently being used is the same. So hipcc in another install location could be expecting a different HCC version than the hipcc in the path relative to the current libhiprtc.so. When that happens, hipcc will throw a nice little warning about mismatching HCC versions and abort compilation. So the test will fail.
As regards setting HIP_PATH, https://cmake.org/cmake/help/latest/command/add_test.html does not allow that. So I need to set the path manually before I build HIP itself. I don't think that is right either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's nice but irrelevant, since the HIPRTC thing is supposed to work with any hipcc, and the latter either works with the environmental hcc or doesn't. It doesn't expect anything in that regard.

Here's the basic idea, since for some reason it was not crisp enough initially: the .so could be anywhere in the path, with no obvious relationship to the placement of hipcc, and that's a valid use case. You cannot rely on its placement to mean anything, and you should not enshrine any implied expectations by rapport with its meaning.

Finally, just for the sake of argument, it is trivial to set the environment for a test, via set_tests_properties + ENVIRONMENT, see https://cmake.org/cmake/help/latest/command/set_tests_properties.html and https://cmake.org/cmake/help/latest/prop_test/ENVIRONMENT.html.

Change-Id: Ib0ad1f99bc71c03e363e055dd508a7a4a210680a
@mangupta mangupta merged commit ccfb764 into master May 16, 2019
@AlexVlx AlexVlx deleted the feature_hiprtc branch July 9, 2019 12:12
@jedbrown jedbrown mentioned this pull request Sep 18, 2019
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants