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

Conversation

Projects
None yet
3 participants
@AlexVlx
Copy link
Collaborator

commented May 10, 2019

No description provided.

@AlexVlx AlexVlx requested review from bensander, iotamudelta and mangupta May 10, 2019

@iotamudelta
Copy link
Collaborator

left a comment

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

@mangupta
Copy link
Collaborator

left a comment

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

@iotamudelta
Copy link
Collaborator

left a comment

Works after the changes to accommodate #1077

[dtests] Replace RUN -> TEST in hiprtc tests
Change-Id: Ie499e92dfe4e5c94634b1c2b76cf52d241bcfea3
if (p->compiled) return HIPRTC_ERROR_COMPILATION;

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

This comment has been minimized.

Copy link
@mangupta

mangupta May 15, 2019

Collaborator

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"}};

This comment has been minimized.

Copy link
@AlexVlx

AlexVlx May 15, 2019

Author Collaborator

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.

This comment has been minimized.

Copy link
@mangupta

mangupta May 16, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@AlexVlx

AlexVlx May 16, 2019

Author Collaborator

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.

[hit] Set HIP_PATH to HIP_ROOT_DIR for all tests
Change-Id: Ib0ad1f99bc71c03e363e055dd508a7a4a210680a

@mangupta mangupta merged commit ccfb764 into master May 16, 2019

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.