[PATCH] D82930: [HIP] Fix rocm detection

[Artem Belevich via Phabricator via cfe-commits]

tra accepted this revision.
tra added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: dexonsmith.

LGTM in principle. Few style comments.



================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:161
+    unsigned Minor = 0;
+    auto Splits = HIPVersionArg.split('.');
+    Splits.first.getAsInteger(0, Major);
----------------
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.


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:244-257
+    llvm::sys::path::append(LibDevicePath, "amdgcn", "bitcode");
+    HasDeviceLibrary = CheckDeviceLib();
+    if (HasDeviceLibrary)
+      return;
+    LibDevicePath = InstallPath;
+    llvm::sys::path::append(LibDevicePath, "lib");
+    HasDeviceLibrary = CheckDeviceLib();
----------------
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;
     }
  }
```


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:248-249
+      return;
+    LibDevicePath = InstallPath;
+    llvm::sys::path::append(LibDevicePath, "lib");
+    HasDeviceLibrary = CheckDeviceLib();
----------------
tra wrote:
> `CheckDeviceLib` should probably take the path to check as an argument and so should `scanLibDevicePath`. 
> 
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.



================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:248-250
+    LibDevicePath = InstallPath;
+    llvm::sys::path::append(LibDevicePath, "lib");
+    HasDeviceLibrary = CheckDeviceLib();
----------------
`CheckDeviceLib` should probably take the path to check as an argument and so should `scanLibDevicePath`. 



================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:299
                                                  ArgStringList &CC1Args) const {
+  bool IsRocm35 = VersionMajorMinor <= llvm::VersionTuple(3, 5);
+
----------------
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`.


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:333
+  if (!IsRocm35) {
+    CC1Args.push_back("-include");
+    CC1Args.push_back("__clang_hip_runtime_wrapper.h");
----------------
You could use `CC1Args.append({"foo", "bar"})` here.


================
Comment at: clang/lib/Driver/ToolChains/HIP.cpp:283
   // Find in --hip-device-lib-path and HIP_LIBRARY_PATH.
-  for (auto Path :
-       DriverArgs.getAllArgValues(options::OPT_hip_device_lib_path_EQ))
-    LibraryPaths.push_back(DriverArgs.MakeArgString(Path));
+  for (auto P : RocmInstallation.getRocmDeviceLibPathArg())
+    LibraryPaths.push_back(DriverArgs.MakeArgString(P));
----------------
Nit: `Path` was fine.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82930/new/

https://reviews.llvm.org/D82930