[PATCH] D22518: Refactor how include paths are appended to the command arguments.

[Samuel Antao via cfe-commits]

sfantao added a comment.

Hi Art, Richard,

Thanks for looking at this patch. I tried to address Richard concerns in the last diff.

Let me know your thoughts,

Thanks again,
Samuel


================
Comment at: lib/Driver/Tools.cpp:308-335
@@ -349,1 +307,30 @@
+    Action::OffloadKind ActiveOffloadingKind = Action::OFK_None) {
+  SmallVector<const ToolChain *, 3> RelevantToolChains;
+  // Add the current tool chain to the relevant tool chain list if it is
+  // defined.
+  if (RegularToolChain)
+    RelevantToolChains.push_back(RegularToolChain);
+
+  // Add all the offloading tool chains associated with the current action to
+  // the relevant tool chain list. If we don't have a specific active offload
+  // kind, consider all available, otherwise consider only the active kind.
+  if (ActiveOffloadingKind == Action::OFK_None ||
+      ActiveOffloadingKind == Action::OFK_Cuda) {
+    if (JA.isHostOffloading(Action::OFK_Cuda))
+      RelevantToolChains.push_back(
+          C.getSingleOffloadToolChain<Action::OFK_Host>());
+    else if (JA.isDeviceOffloading(Action::OFK_Cuda))
+      RelevantToolChains.push_back(
+          C.getSingleOffloadToolChain<Action::OFK_Cuda>());
+  }
+
+  //
+  // TODO: Add support for other offloading programming models here.
+  //
+
+  // Apply Work on all the relevant tool chains.
+  for (const auto *TC : RelevantToolChains) {
+    assert(TC && "Undefined tool chain??");
+    Work(TC);
+  }
 }
----------------
rsmith wrote:
> There's no point in building a `SmallVector` here, just directly call `Work` when you find a toolchain that should be used.
Ok, I'm doing as you suggest.

================
Comment at: lib/Driver/Tools.cpp:317-318
@@ +316,4 @@
+  // kind, consider all available, otherwise consider only the active kind.
+  if (ActiveOffloadingKind == Action::OFK_None ||
+      ActiveOffloadingKind == Action::OFK_Cuda) {
+    if (JA.isHostOffloading(Action::OFK_Cuda))
----------------
rsmith wrote:
> What is this `ActiveOffloadingKind` parameter for? Both values that we actually pass in here do the exact same thing.
`ActiveOffloadingKind` was meant to signal the offloading model in use. Its true right now we only have CUDA kind. We are proposing the OpenMP kind in https://reviews.llvm.org/D21843 (one of the depending patches of the patch that inserted this modification), so the goal was to pave the way for that.

In any case, I'm removing the `ActiveOffloadingKind` check, as I am calling `AddCudaIncludeArgs` directly in the caller of this function. Let me know if you'd like to fix this in a different way.

================
Comment at: lib/Driver/Tools.cpp:341-346
@@ -340,8 @@
-
-  if (JA.isHostOffloading(Action::OFK_Cuda))
-    C.getSingleOffloadToolChain<Action::OFK_Host>()->AddCudaIncludeArgs(
-        Args, CmdArgs);
-  else if (JA.isDeviceOffloading(Action::OFK_Cuda))
-    C.getSingleOffloadToolChain<Action::OFK_Cuda>()->AddCudaIncludeArgs(
-        Args, CmdArgs);
-
----------------
rsmith wrote:
> The OFK_Host / OFK_Cuda arguments here are reversed from the other two cases. Is that a bug that's fixed by this change, or a bug that's introduced by this change? :)
> 
> Either way it seems that we're missing test coverage.
As per the implementation before my change, `AddCudaIncludeArgs` should be called on the current toolchain. The outcome of using one toolchain or the other is similar except that for a CUDA toolchain there is an extra check. So, we'd always get the check if producing commands for both toolchains except if compiling with host/device-only mode. I fixed the issue here and added regression tests for host/device only . 


https://reviews.llvm.org/D22518