[HIP] Fix rocm detection

[yaxunl]

Summary

Do not detect device library by default in rocm detector. Only detect device library in HIP toolchain.

Detect rocm path by version file in host toolchains.

Also added detecting rocm version and printing rocm installation path and version with -v.

Fixed include path and device library detection for ROCm 3.5.

[arsenm]

clang/lib/Driver/ToolChains/AMDGPU.cpp

167

I don't think the detection should fail if this is missing

[yaxunl]

clang/lib/Driver/ToolChains/AMDGPU.cpp

167

why? this file is unique to HIP installation. If it is missing, the installation is broken.

[arsenm]

clang/lib/Driver/ToolChains/AMDGPU.cpp

167

Because I should be able to use this without a complete hip installation. Without a specified version, it should assume the most modern layout. This will for example break pointing --rocm-path at the device library build directory for library tests

[arsenm]

clang/lib/Driver/ToolChains/AMDGPU.cpp

167

I also don't see what value checking the version really provides; it may be informative to print it, but I don't think it's useful to derive information from it

[yaxunl]

clang/lib/Driver/ToolChains/AMDGPU.cpp

167

what is the directory structure of your most modern layout?

[arsenm]

clang/lib/Driver/ToolChains/AMDGPU.cpp

167

/opt/rocm/amdgcn/bitcode/foo.bc

The plan is to remove this and rely on symlinks in the resource directory though

[tra]

clang/lib/Driver/ToolChains/AMDGPU.cpp

167

I also don't see what value checking the version really provides; it may be informative to print it, but I don't think it's useful to derive information from it

In CUDA it's used to detect which back-end features to enable (they depend on what's supported by ptxas supplied by that CUDA version). I don't think that would be relevant to AMDGPU as it does not need external dependencies to generate GPU code.

It may be useful for filesystem layout detection. E.g. where to find bitcode files and what names to expect. This part seems to be relevant for ROCm, but I'm not sure if the layout is closely tied to the version.

[yaxunl]

clang/lib/Driver/ToolChains/AMDGPU.cpp

167

We are required to support previous ROCm releases for certain time range. To do that we need to detect HIP version and enable/disable certain HIP features based on HIP version when necessary.

Therefore if we have a new directory structure for ROCm, that directory structure should contain a HIP version file so that we can detect HIP version.

[yaxunl]

clang/lib/Driver/ToolChains/AMDGPU.cpp

167

Currently clang includes some wrapper headers by default, which does not work with ROCm 3.5 since those device functions are defined in HIP headers of ROCm 3.5. To support ROCm 3.5, we have to disable including those wrapper headers. This is one example why we need detect HIP version.

167

I think we need to separate HIP runtime detection and device library detection and also separate hasValidHIPRuntime and hasValidDeviceLibrary. ROCm toolchain only need to make sure hasValidDeviceLibrary but HIP toolchain need both hasValidDeviceLibrary and hasValidHIPRuntime.

[arsenm]

clang/lib/Driver/ToolChains/AMDGPU.cpp

167

Regardless of whether there's a version file or if does anything, I think the absence of one implies do the most modern thing

[tra]

clang/lib/Driver/ToolChains/AMDGPU.cpp

167

"The most modern thing" is an ever moving target. If the failure modes keep changing it will create a source of new problems every time something changes. Probably not a big issue in this case, but with (eventually) wide enough user base there will be some.

I assume that AMD does not have much of legacy user base yet for compilations with clang, so defaulting to a recent version may be sensible (unlike CUDA where clang had to deal with a lot of existing CUDA users using old CUDA versions). That said, I'd recommend pinning it to a specific version, so the expectations remain stable. Then we can document the failure/fallback in a way users can deal with -- if no version file is found, clang assumes version x.y and expects to find files X, Y and Z in directory A, B, C. You can specify the locations manually with --something-something-X=A or specify the version with --hip-version=3.7.

Considering that clang is being used from various derived tools, you may not always have the luxury of having the whole ROCm installation around and need to be able to override the expectations via command line flags.

If we have a way to override the version, then it does not matter all that much which version we pick as a fallback. If the guess is wrong, we can always correct it with a flag.

[arsenm]

clang/lib/Driver/ToolChains/AMDGPU.cpp

167

Unlike CUDA we're not really chasing some 3rd party thing that periodically drops unpredictable changes. We know what the most modern thing is on tip of tree

[tra]

clang/lib/Driver/ToolChains/AMDGPU.cpp

167

Unlike CUDA we're not really chasing some 3rd party thing that periodically drops unpredictable changes. We know what the most modern thing is on tip of tree

We're in agreement on this part. If lack of the version file is non-fatal, I'm fine with whatever version AMD picks as the default.

I'm just pointing out that you'll likely need a way to keep compiler working even if the filesystem does not have everything you expect for the full end-to-end compilation, be it the version file or bitcode. Reasonable default is fine. Overrides can be implemented later, if necessary.

[yaxunl]

Separate detecting ROCm path and device library path.

Assume 3.5 as default HIP version. If -rocm-path is specified, do not require version file to exist.

Added --hip-version to override detected HIP version.

Fixed default option for -std and -fhip-use-new-launch-api.

[tra]

LGTM in principle. Few style comments.

clang/lib/Driver/ToolChains/AMDGPU.cpp

172

A comment about expected version format would be helpful here.

Perhaps it could be simplified by splitting the string on "." into an array in one call instead of doing it one element at a time.

226–239

It would be nice to collect the list of directories to check into an array and then loop over it -- the same way you already do for the InstallPath candidates. Makes it easier to add/change search logic.

E.g. in the end we would get something like this:

for (path: {InstallPath, make_path(InstallPath, "lib"), make_path(InstallPath, "lib", "bitcode"), ... }) {
   if (CheckDeviceLib(path)) {
      // record path as the correct location
     return;
   }
}

230–231

Creation of the path from base+suffixes seems to be a common pattern to be extracted into a lambda. I'm surprised it's not provided by LLVM itself.

230–232

CheckDeviceLib should probably take the path to check as an argument and so should scanLibDevicePath.

250

Nit: IsRocm35 is a bit misleading as it would be true for versions other than 3.5.
Rename it to something closer matching the intent? IsRocm35OrOlder ot, perhaps, hipUsesRuntimeWrapper.

282

You could use CC1Args.append({"foo", "bar"}) here.

clang/lib/Driver/ToolChains/HIP.cpp

283 ↗

(On Diff #277095)

Nit: Path was fine.

[yaxunl]

thanks. will fix when commit

[njames93]

This is causing a test case to fail on mac http://45.33.8.238/mac/17048/step_7.txt

[thakis]

This breaks check-clang on macOS: http://45.33.8.238/mac/17053/step_7.txt

The output contains

5: clang: warning: argument unused during compilation: '--rocm-path=/Users/thakis/src/llvm-project/clang/test/Driver/Inputs/rocm' [-Wunused-command-line-argument]

which if true would explain the failure.

That bot uses the GN build, but I verified that the test fails the same way in the CMake build.

Please take a look, and if it takes a while to fix, please revert while you investigate.

clang/test/Driver/Inputs/rocm/bin/.hipVersion

2

Is this true? I don't see any cmake code generating this. Did I miss it?

[thakis]

…oh, njames said that already 5h ago :) I guess I'll wait another hour or two, and then I'll revert.

[yaxunl]

In D82930#2145914, @thakis wrote:

This breaks check-clang on macOS: http://45.33.8.238/mac/17053/step_7.txt

The output contains

5: clang: warning: argument unused during compilation: '--rocm-path=/Users/thakis/src/llvm-project/clang/test/Driver/Inputs/rocm' [-Wunused-command-line-argument]

which if true would explain the failure.

That bot uses the GN build, but I verified that the test fails the same way in the CMake build.

Please take a look, and if it takes a while to fix, please revert while you investigate.

should be fixed by 5d2c3e031a6861b3e95673d0e238c09938dd9c0d

[yaxunl]

In D82930#2145827, @njames93 wrote:

This is causing a test case to fail on mac http://45.33.8.238/mac/17048/step_7.txt

5d2c3e031a6861b3e95673d0e238c09938dd9c0d should fix it

[thakis]

Great, thanks :)

clang/lib/Driver/ToolChains/AMDGPU.cpp

if (BaseName == "ocml") {
       llvm::Twine GfxName = Twine("gfx") + IsaVersionNumber;
       SmallString<8> Tmp;
       LibDeviceMap.insert(
         std::make_pair(GfxName.toStringRef(Tmp), FilePath.str()));
     }
   }
 }
 
+void RocmInstallationDetector::ParseHIPVersionFile(llvm::StringRef V) {
+  SmallVector<StringRef, 4> VersionParts;
+  V.split(VersionParts, '\n');
+  unsigned Major;
+  unsigned Minor;
+  for (auto Part : VersionParts) {
+    auto Splits = Part.split('=');
+    if (Splits.first == "HIP_VERSION_MAJOR")
+      Splits.second.getAsInteger(0, Major);
+    else if (Splits.first == "HIP_VERSION_MINOR")
+      Splits.second.getAsInteger(0, Minor);
+    else if (Splits.first == "HIP_VERSION_PATCH")
+      VersionPatch = Splits.second.str();
+  }
+  VersionMajorMinor = llvm::VersionTuple(Major, Minor);
+  DetectedVersion =
+      (Twine(Major) + "." + Twine(Minor) + "." + VersionPatch).str();
+}
+
 RocmInstallationDetector::RocmInstallationDetector(
     const Driver &D, const llvm::Triple &HostTriple,
     const llvm::opt::ArgList &Args)
     : D(D) {
   struct Candidate {
     std::string Path;
     bool StrictChecking;
 
if (ParentName == "llvm") {
       // Some versions of the rocm llvm package install to /opt/rocm/llvm/bin
       Candidates.emplace_back(llvm::sys::path::parent_path(ParentDir).str(),
                               /*StrictChecking=*/true);
     }
 
     Candidates.emplace_back(D.SysRoot + "/opt/rocm");
   }
 
+  auto &FS = D.getVFS();
+
+  for (const auto &Candidate : Candidates) {
+    InstallPath = Candidate.Path;
+    if (InstallPath.empty() || !FS.exists(InstallPath))
+      continue;
+
+    llvm::sys::path::append(BinPath, InstallPath, "bin");
+    llvm::sys::path::append(IncludePath, InstallPath, "include");
+    llvm::sys::path::append(LibPath, InstallPath, "lib");
+
+    llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> VersionFile =
+        FS.getBufferForFile(BinPath + "/.hipVersion");
+    if (!VersionFile)
+      continue;
+
+    ParseHIPVersionFile((*VersionFile)->getBuffer());
+    IsValid = true;
+    return;
+  }
+  IsValid = false;
+}
+
+void RocmInstallationDetector::detectDeviceLibrary(
+    const llvm::opt::ArgList &Args) {
+  if (!IsValid)
+    return;
   bool NoBuiltinLibs = Args.hasArg(options::OPT_nogpulib);
 
   assert(LibDevicePath.empty());
 
   if (Args.hasArg(clang::driver::options::OPT_hip_device_lib_path_EQ)) {
     LibDevicePath
       = Args.getLastArgValue(clang::driver::options::OPT_hip_device_lib_path_EQ);
   } else if (const char *LibPathEnv = ::getenv("HIP_DEVICE_LIB_PATH")) {
     LibDevicePath = LibPathEnv;
   }
 
   auto &FS = D.getVFS();
   if (!LibDevicePath.empty()) {
     // Maintain compatability with HIP flag/envvar pointing directly at the
     // bitcode library directory. This points directly at the library path instead
     // of the rocm root installation.
     if (!FS.exists(LibDevicePath))
       return;
 
     scanLibDevicePath();
     IsValid = allGenericLibsValid() && !LibDeviceMap.empty();
     return;
   }
 
-  for (const auto &Candidate : Candidates) {
-    InstallPath = Candidate.Path;
-    if (InstallPath.empty() || !FS.exists(InstallPath))
-      continue;
-
-    // The install path situation in old versions of ROCm is a real mess, and
-    // use a different install layout. Multiple copies of the device libraries
-    // exist for each frontend project, and differ depending on which build
-    // system produced the packages. Standalone OpenCL builds also have a
-    // different directory structure from the ROCm OpenCL package.
-    //
-    // The desired structure is (${ROCM_ROOT} or
-    // ${OPENCL_ROOT})/amdgcn/bitcode/*, so try to detect this layout.
-
-    // BinPath = InstallPath + "/bin";
-    llvm::sys::path::append(IncludePath, InstallPath, "include");
-    llvm::sys::path::append(LibDevicePath, InstallPath, "amdgcn", "bitcode");
+  // The desired structure is ${ROCM_ROOT})/amdgcn/bitcode/* or
+  // ${ROCM_ROOT}/lib/* so try to detect this layout.
 
-    // We don't need the include path for OpenCL, since clang already ships with
-    // the default header.
-
-    bool CheckLibDevice = (!NoBuiltinLibs || Candidate.StrictChecking);
+  auto CheckDeviceLib = [&]() {
+    bool CheckLibDevice = !NoBuiltinLibs;
     if (CheckLibDevice && !FS.exists(LibDevicePath))
-      continue;
+      return false;
 
     scanLibDevicePath();
 
     if (!NoBuiltinLibs) {
       // Check that the required non-target libraries are all available.
       if (!allGenericLibsValid())
-        continue;
+        return false;
 
       // Check that we have found at least one libdevice that we can link in if
       // -nobuiltinlib hasn't been specified.
       if (LibDeviceMap.empty())
-        continue;
-    }
-
-    IsValid = true;
-    break;
+        return false;
     }
+    return true;
+  };
+  llvm::sys::path::append(LibDevicePath, InstallPath, "amdgcn", "bitcode");
+  IsValid = CheckDeviceLib();
+  if (IsValid)
+    return;
+  llvm::sys::path::append(LibDevicePath, InstallPath, "lib");
+  IsValid = CheckDeviceLib();
 }
 
 void RocmInstallationDetector::print(raw_ostream &OS) const {
   if (isValid())
-    OS << "Found ROCm installation: " << InstallPath << '\n';
+    OS << "Found ROCm installation: " << InstallPath << ", version "
+       << DetectedVersion << '\n';
 }
 
 void RocmInstallationDetector::AddHIPIncludeArgs(const ArgList &DriverArgs,
                                                  ArgStringList &CC1Args) const {
+  if (VersionMajorMinor <= llvm::VersionTuple(3, 5))
+    return;
   if (!DriverArgs.hasArg(options::OPT_nobuiltininc)) {
     // HIP header includes standard library wrapper headers under clang
     // cuda_wrappers directory. Since these wrapper headers include_next
     // standard C++ headers, whereas libc++ headers include_next other clang
     // headers. The include paths have to follow this order:
     // - wrapper include path
     // - standard C++ include path
     // - other clang include path
if (!isValid()) {
     D.Diag(diag::err_drv_no_rocm_installation);
     return;
   }
 
   CC1Args.push_back("-internal-isystem");
   CC1Args.push_back(DriverArgs.MakeArgString(getIncludePath()));
   CC1Args.push_back("-include");
   CC1Args.push_back("__clang_hip_runtime_wrapper.h");
 }
 
 void amdgpu::Linker::ConstructJob(Compilation &C, const JobAction &JA,
                                   const InputInfo &Output,
                                   const InputInfoList &Inputs,
                                   const ArgList &Args,
                                   const char *LinkingOutput) const {
 
   std::string Linker = getToolChain().GetProgramPath(getShortName());
bool AMDGPUToolChain::isWave64(const llvm::opt::ArgList &DriverArgs,
   return !HasWave32 || DriverArgs.hasFlag(
     options::OPT_mwavefrontsize64, options::OPT_mno_wavefrontsize64, false);
 }
 
 
 /// ROCM Toolchain
 ROCMToolChain::ROCMToolChain(const Driver &D, const llvm::Triple &Triple,
                              const ArgList &Args)
-  : AMDGPUToolChain(D, Triple, Args),
-    RocmInstallation(D, Triple, Args) { }
+    : AMDGPUToolChain(D, Triple, Args), RocmInstallation(D, Triple, Args) {
+  RocmInstallation.detectDeviceLibrary(Args);
+}
 
 void AMDGPUToolChain::addClangTargetOptions(
     const llvm::opt::ArgList &DriverArgs,
     llvm::opt::ArgStringList &CC1Args,
     Action::OffloadKind DeviceOffloadingKind) const {
   // Default to "hidden" visibility, as object level linking will not be
   // supported for the foreseeable future.
   if (!DriverArgs.hasArg(options::OPT_fvisibility_EQ,

clang/lib/Driver/ToolChains/Gnu.cpp

 }
 
 Tool *Generic_GCC::buildLinker() const { return new tools::gcc::Linker(*this); }
 
 void Generic_GCC::printVerboseInfo(raw_ostream &OS) const {
   // Print the information about how we detected the GCC installation.
   GCCInstallation.print(OS);
   CudaInstallation.print(OS);
+  RocmInstallation.print(OS);
 }
 
 bool Generic_GCC::IsUnwindTablesDefault(const ArgList &Args) const {
   return getArch() == llvm::Triple::x86_64;
 }
 
 bool Generic_GCC::isPICDefault() const {
   switch (getArch()) {

clang/lib/Driver/ToolChains/MSVC.cpp

 
 void MSVCToolChain::AddHIPIncludeArgs(const ArgList &DriverArgs,
                                       ArgStringList &CC1Args) const {
   RocmInstallation.AddHIPIncludeArgs(DriverArgs, CC1Args);
 }
 
 void MSVCToolChain::printVerboseInfo(raw_ostream &OS) const {
   CudaInstallation.print(OS);
+  RocmInstallation.print(OS);
 }
 
 // Windows SDKs and VC Toolchains group their contents into subdirectories based
 // on the target architecture. This function converts an llvm::Triple::ArchType
 // to the corresponding subdirectory name.
 static const char *llvmArchToWindowsSDKArch(llvm::Triple::ArchType Arch) {
   using ArchType = llvm::Triple::ArchType;
   switch (Arch) {

clang/lib/Driver/ToolChains/ROCm.h

 #include "clang/Basic/LLVM.h"
 #include "clang/Driver/Driver.h"
 #include "clang/Driver/Options.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Option/ArgList.h"
+#include "llvm/Support/VersionTuple.h"
 
 namespace clang {
 namespace driver {
 
 /// A class to find a viable ROCM installation
 /// TODO: Generalize to handle libclc.
 class RocmInstallationDetector {
 private:
   struct ConditionalLibrary {
     SmallString<0> On;
     SmallString<0> Off;
 
     bool isValid() const { return !On.empty() && !Off.empty(); }
 
     StringRef get(bool Enabled) const {
       assert(isValid());
       return Enabled ? On : Off;
     }
   };
 
+
   const Driver &D;
   bool IsValid = false;
-  // RocmVersion Version = RocmVersion::UNKNOWN;
+  // The version string in Major.Minor.Path format.
+  std::string DetectedVersion;
+  // Version containing major and minor.
+  llvm::VersionTuple VersionMajorMinor;
+  // Version containing patch.
+  std::string VersionPatch;
   SmallString<0> InstallPath;
-  // SmallString<0> BinPath;
+  SmallString<0> BinPath;
   SmallString<0> LibPath;
   SmallString<0> LibDevicePath;
   SmallString<0> IncludePath;
   llvm::StringMap<std::string> LibDeviceMap;
 
   // Libraries that are always linked.
   SmallString<0> OCML;
   SmallString<0> OCKL;
return !OCML.empty() && !OCKL.empty() && !OpenCL.empty() && !HIP.empty() &&
            CorrectlyRoundedSqrt.isValid();
   }
 
   // GPU architectures for which we have raised an error in
   // CheckRocmVersionSupportsArch.
   mutable llvm::SmallSet<CudaArch, 4> ArchsWithBadVersion;
 
   void scanLibDevicePath();
+  void ParseHIPVersionFile(llvm::StringRef V);
 
 public:
   RocmInstallationDetector(const Driver &D, const llvm::Triple &HostTriple,
                            const llvm::opt::ArgList &Args);
 
   /// Add arguments needed to link default bitcode libraries.
   void addCommonBitcodeLibCC1Args(const llvm::opt::ArgList &DriverArgs,
                                   llvm::opt::ArgStringList &CC1Args,
public:
 
   /// Get libdevice file for given architecture
   std::string getLibDeviceFile(StringRef Gpu) const {
     return LibDeviceMap.lookup(Gpu);
   }
 
   void AddHIPIncludeArgs(const llvm::opt::ArgList &DriverArgs,
                          llvm::opt::ArgStringList &CC1Args) const;
+
+  void detectDeviceLibrary(const llvm::opt::ArgList &Args);
 };
 
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_ROCM_H

clang/test/Driver/Inputs/rocm/bin/.hipVersion

+# Auto-generated by cmake
+HIP_VERSION_MAJOR=3
+HIP_VERSION_MINOR=6
+HIP_VERSION_PATCH=20214-a2917cd