[PATCH] D116627: [Clang] Initial support for linking offloading code in tool

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Same as the other commit, where is this tested?



================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:425
+  ErrorOr<std::string> NvlinkPath = sys::findProgramByName(
+      "nvlink", sys::path::parent_path(LinkerExecutable));
+  if (!NvlinkPath)
----------------
Unsure why we look at the linker bin dir first, PATH seems to be the right choice here.


================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:439
+
+  // TODO: Pass in arguments like `-g` and `-v` from the driver.
+  SmallVector<StringRef, 16> CmdArgs;
----------------
If this is not addressed in a follow up, it seems trivial to add it.


================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:468
-
-  return EXIT_SUCCESS;
 }
----------------
keep exit success, makes it obvious this worked.


================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:497
+    Triple TheTriple(TargetFeatures.first);
+    StringRef Arch(TargetFeatures.second);
+
----------------
.str() above to get a key is somewhat ok, but then parsing the key again is weird.
Can we make it a densemap and provide a densemapinfo for `struct DeviceFile`. It can just inherit from the DenseMapInfo<std::string>. Might even work w/o explicitly calling the "super" functions with the `.str()` version if we have a `std::string operator`. Might then even work w/o DenseMapInfo by telling the DenseMap to use the std::string specialization of DenseMapInfo in the first place.


================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:633
+  for (const auto &Image : LinkedImages) {
+    auto FileOrErr = wrapDeviceImage(Image);
+    if (!FileOrErr)
----------------
Try to replace auto where the type is short and helpful. E.g., here it would help to know what it is.


================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:647
     if (std::error_code EC = sys::fs::remove(TempFile))
-      reportError(createFileError(TempFile, EC));
-  }
-
-  return EXIT_SUCCESS;
+      return reportError(createFileError(TempFile, EC));
 }
----------------
do we really want to return here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116627