[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added inline comments.


================
Comment at: lib/Driver/ToolChains/Cuda.cpp:388
+
+  // add paths specified in LIBRARY_PATH environment variable as -L options.
+  addDirectoryList(Args, CmdArgs, "-L", "LIBRARY_PATH");
----------------
Comment sentences should start with a capital letter (here and below).


================
Comment at: lib/Driver/ToolChains/Cuda.cpp:407
+  // the target details to the driver and maybe we do not want to do
+  // that
+  for (const auto &II : Inputs) {
----------------
Add period at the end of the sentence.


================
Comment at: lib/Driver/ToolChains/Cuda.cpp:409
+  for (const auto &II : Inputs) {
+
+    if (II.getType() == types::TY_LLVM_IR ||
----------------
Remove this blank line.


================
Comment at: lib/Driver/ToolChains/Cuda.cpp:420
+    // Currently, we only pass the input files to the linker, we do not pass
+    // any libraries that may be valid only for the host.
+    if (!II.isFilename())
----------------
Can you please explain how this works for libraries specified by file name (e.g. /path/to/foo.so or /path/to/foo.a) or why these should be treated differently? Maybe I'm misunderstanding what this check does.


Repository:
  rL LLVM

https://reviews.llvm.org/D29654