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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
