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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added inline comments.


================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:425
+  ErrorOr<std::string> NvlinkPath = sys::findProgramByName(
+      "nvlink", sys::path::parent_path(LinkerExecutable));
+  if (!NvlinkPath)
----------------
jdoerfert wrote:
> Unsure why we look at the linker bin dir first, PATH seems to be the right choice here.
Copied it from somewhere, should change it. Right now it's just a weird way to look through `/bin/`


================
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;
----------------
jdoerfert wrote:
> If this is not addressed in a follow up, it seems trivial to add it.
It is addressed later.


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


================
Comment at: clang/tools/clang-linker-wrapper/ClangLinkerWrapper.cpp:497
+    Triple TheTriple(TargetFeatures.first);
+    StringRef Arch(TargetFeatures.second);
+
----------------
jdoerfert wrote:
> .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.
Probably a smarter idea, I needed a way to group all files with the same device file type. I'll look into it.


================
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));
 }
----------------
jdoerfert wrote:
> do we really want to return here?
Might be a good idea to keep trying to remove temp files even if one fails, will address.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116627