[PATCH] D47394: [OpenMP][Clang][NVPTX] Replace bundling with partial linking for the OpenMP NVPTX device offloading toolchain

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added subscribers: xtian, gregrodgers, ddibyend.
jdoerfert added a comment.
Herald added a subscriber: ormris.

Could you sketch for me how this will (potentially) work if we have multiple target vendors? The fatbin solution seems tailored to NVIDIA, but maybe I'm wrong here.

In any case, we need to make progress on this front and if this solution is compatible with other vendors we should get it in asap.

@xtian, @gregrodgers, @ddibyend please take a look or have someone take a look and comment.



================
Comment at: lib/Driver/Driver.cpp:3972
   bool BuildingForOffloadDevice = TargetDeviceOffloadKind != Action::OFK_None;
+
   if (const OffloadAction *OA = dyn_cast<OffloadAction>(A)) {
----------------
unrelated


================
Comment at: lib/Driver/ToolChains/Clang.cpp:6117
+    CmdArgs.push_back(TCArgs.MakeArgString(Inputs[I].getFilename()));
+  }
+
----------------
In "core-LLVM" we usually avoid these braces.


================
Comment at: lib/Driver/ToolChains/Cuda.cpp:401
+  const char *CubinF = Args.MakeArgString(TC.getInputFilename(Output));
+  CmdArgs.push_back(CubinF);
   for (const auto& II : Inputs)
----------------
It might not be worth it to save CubinF here but create it 120 lines later instead


================
Comment at: lib/Driver/ToolChains/Cuda.cpp:547
+    const char *CompilerExec =
+        Args.MakeArgString(TC.GetProgramPath("clang++"));
+    C.addCommand(llvm::make_unique<Command>(
----------------
You cannot hardcode clang++, it could be C code and we don't want to cause interoperability problems and/or the warnings that will inevitably follow.


================
Comment at: lib/Driver/ToolChains/Cuda.cpp:661
+  if (C.canSkipOffloadBundler())
+    Args.AddAllArgs(CmdArgs, options::OPT_L);
+
----------------
Could you add a comment here please?


================
Comment at: lib/Driver/ToolChains/Cuda.cpp:687
+        CmdArgs.push_back(C.getArgs().MakeArgString(llvm::Twine("-l") +
+              II.getInputArg().getValue()));
       continue;
----------------
By comparing this code with the one after the `if (... endwith(".a"))` it seems this treated a bit differently than a static library below. I mention it only because of the comment above.


Repository:
  rC Clang

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

https://reviews.llvm.org/D47394