This is an archive of the discontinued LLVM Phabricator instance.

[HIP] Fix version detection for old HIP-PATH
ClosedPublic

Authored by yaxunl on Jun 29 2023, 6:11 AM.

Details

Summary

ROCm used to install components under individual directories,
e.g. HIP installed to /opt/rocm/hip and rocblas installed to
/opt/rocm/rocblas. ROCm has transitioned to a flat directory
structure where all components are installed to /opt/rocm.
HIP-PATH and --hip-path are supposed to be /opt/rocm as
clang detect HIP version by /opt/rocm/share/hip/version.
However, some existing HIP app still uses HIP-PATH=/opt/rocm/hip.
To avoid regression, clang will also try detect share/hip/version
under the parent directory of HIP-PATH or --hip-path.
This way, the detection will work for both new HIP-PATH and
old HIP-PATH.

Diff Detail

Event Timeline

yaxunl created this revision.Jun 29 2023, 6:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2023, 6:11 AM
yaxunl requested review of this revision.Jun 29 2023, 6:11 AM
tra accepted this revision.Jun 29 2023, 10:54 AM

LGTM in general with a minor suggestion.

clang/lib/Driver/ToolChains/AMDGPU.cpp
471

We seem to be rather inconsistent about how we handle paths.

Above, we use llvm::sys::path::append, but here we revert to just appending a path as a string.

I think we should be using llvm::sys::path API consistently. It's unfortunate that the API does not provide a string-returning function to append elements.

auto Append = [](SmallVectorImpl<char> &path, const Twine &a,
                                         const Twine &b = "",
                                         const Twine &c = "",
                                         const Twine &d = "") {
    SmallVectorImpl<char> newpath = path;
    llvm::sys::path::append(newpath, a,b,c,d);
    return newpath; 
}
for (const auto &VersionFilePath :
         {Append(SharePath, "hip", "version"),
          Append(ParentSharePath,  "hip", "version"),
          Append(BinPath, ".hipVersion")}) {
  ...
}
This revision is now accepted and ready to land.Jun 29 2023, 10:54 AM
yaxunl marked an inline comment as done.Jun 29 2023, 11:28 AM
yaxunl added inline comments.
clang/lib/Driver/ToolChains/AMDGPU.cpp
471

will do. thanks.

This revision was landed with ongoing or failed builds.Jun 29 2023, 11:57 AM
This revision was automatically updated to reflect the committed changes.
yaxunl marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2023, 11:57 AM