Refactor how include paths are appended to the command arguments.

[sfantao]

Summary

This patch aims at removing redundancy in the way include paths for the regular and offloading toolchains are appended to the arguments list in the clang tool.

This was suggested by @rsmith in response to r275931.

[tra]

Looks good.

[rsmith]

lib/Driver/Tools.cpp

324–351

There's no point in building a SmallVector here, just directly call Work when you find a toolchain that should be used.

333–334

What is this ActiveOffloadingKind parameter for? Both values that we actually pass in here do the exact same thing.

341–346

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.

[sfantao]

• Remove of use of SmallVector and include CUDA args only for the regular toolchain.

[sfantao]

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

lib/Driver/Tools.cpp

324–351

Ok, I'm doing as you suggest.

333–334

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.

341–346

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 .

[tra]

Looks good.

[tra]

Samuel, the patch breaks CUDA. With the patch clang no longer adds include paths to standard c++ library during device-side compilation.
if you run "clang++ -### -c -x cuda /dev/null" you will see that host side gets "-internal-isystem" "/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/x86_64-linux-gnu/c++/4.8" but device-side compilation does not.

[sfantao]

In D22518#500066, @tra wrote:

Samuel, the patch breaks CUDA. With the patch clang no longer adds include paths to standard c++ library during device-side compilation.
if you run "clang++ -### -c -x cuda /dev/null" you will see that host side gets "-internal-isystem" "/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/x86_64-linux-gnu/c++/4.8" but device-side compilation does not.

Hi Art,

The enumerators were switched... It should be fixed in r277064. I'll post a patch with regression test to catch the issue.

Sorry for the trouble,
Samuel

lib/Driver/Tools.cpp

 
 static bool forwardToGCC(const Option &O) {
   // Don't forward inputs from the original command line.  They are added from
   // InputInfoList.
   return O.getKind() != Option::InputClass &&
          !O.hasFlag(options::DriverOption) && !O.hasFlag(options::LinkerInput);
 }
 
-/// Add the C++ include args of other offloading toolchains. If this is a host
-/// job, the device toolchains are added. If this is a device job, the host
-/// toolchains will be added.
-static void addExtraOffloadCXXStdlibIncludeArgs(Compilation &C,
-                                                const JobAction &JA,
-                                                const ArgList &Args,
-                                                ArgStringList &CmdArgs) {
-
-  if (JA.isHostOffloading(Action::OFK_Cuda))
-    C.getSingleOffloadToolChain<Action::OFK_Cuda>()
-        ->AddClangCXXStdlibIncludeArgs(Args, CmdArgs);
-  else if (JA.isDeviceOffloading(Action::OFK_Cuda))
-    C.getSingleOffloadToolChain<Action::OFK_Host>()
-        ->AddClangCXXStdlibIncludeArgs(Args, CmdArgs);
-
-  // TODO: Add support for other programming models here.
-}
-
-/// Add the C include args of other offloading toolchains. If this is a host
-/// job, the device toolchains are added. If this is a device job, the host
-/// toolchains will be added.
-static void addExtraOffloadClangSystemIncludeArgs(Compilation &C,
-                                                  const JobAction &JA,
-                                                  const ArgList &Args,
-                                                  ArgStringList &CmdArgs) {
-
-  if (JA.isHostOffloading(Action::OFK_Cuda))
-    C.getSingleOffloadToolChain<Action::OFK_Cuda>()->AddClangSystemIncludeArgs(
-        Args, CmdArgs);
-  else if (JA.isDeviceOffloading(Action::OFK_Cuda))
-    C.getSingleOffloadToolChain<Action::OFK_Host>()->AddClangSystemIncludeArgs(
-        Args, CmdArgs);
-
-  // TODO: Add support for other programming models here.
-}
-
-/// Add the include args that are specific of each offloading programming model.
-static void addExtraOffloadSpecificIncludeArgs(Compilation &C,
-                                               const JobAction &JA,
-                                               const ArgList &Args,
-                                               ArgStringList &CmdArgs) {
+/// Apply \a Work on the current tool chain \a RegularToolChain and any other
+/// offloading tool chain that is associated with the current action \a JA.
+static void
+forAllAssociatedToolChains(Compilation &C, const JobAction &JA,
+                           const ToolChain &RegularToolChain,
+                           llvm::function_ref<void(const ToolChain &)> Work) {
+  // Apply Work on the current/regular tool chain.
+  Work(RegularToolChain);
 
+  // Apply Work on all the offloading tool chains associated with the current
+  // action.
   if (JA.isHostOffloading(Action::OFK_Cuda))
-    C.getSingleOffloadToolChain<Action::OFK_Host>()->AddCudaIncludeArgs(
-        Args, CmdArgs);
+    Work(*C.getSingleOffloadToolChain<Action::OFK_Host>());
   else if (JA.isDeviceOffloading(Action::OFK_Cuda))
-    C.getSingleOffloadToolChain<Action::OFK_Cuda>()->AddCudaIncludeArgs(
-        Args, CmdArgs);
+    Work(*C.getSingleOffloadToolChain<Action::OFK_Cuda>());
 
-  // TODO: Add support for other programming models here.
+  //
+  // TODO: Add support for other offloading programming models here.
+  //
 }
 
 void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
                                     const Driver &D, const ArgList &Args,
                                     ArgStringList &CmdArgs,
                                     const InputInfo &Output,
                                     const InputInfoList &Inputs) const {
   Arg *A;
   const bool IsIAMCU = getToolChain().getTriple().isOSIAMCU();
 
   CheckPreprocessingOptions(D, Args);
 
   Args.AddLastArg(CmdArgs, options::OPT_C);
   Args.AddLastArg(CmdArgs, options::OPT_CC);
 
   // Handle dependency file generation.
   if ((A = Args.getLastArg(options::OPT_M, options::OPT_MM)) ||
       (A = Args.getLastArg(options::OPT_MD)) ||
       (A = Args.getLastArg(options::OPT_MMD))) {
     // Determine the output location.
     const char *DepFile;
     if (Arg *MF = Args.getLastArg(options::OPT_MF)) {
       DepFile = MF->getValue();
       C.addFailureResultFile(DepFile, &JA);
     } else if (Output.getType() == types::TY_Dependencies) {
       DepFile = Output.getFilename();
     } else if (A->getOption().matches(options::OPT_M) ||
                A->getOption().matches(options::OPT_MM)) {
       DepFile = "-";
     } else {
       DepFile = getDependencyFileName(Args, Inputs);
       C.addFailureResultFile(DepFile, &JA);
     }
     CmdArgs.push_back("-dependency-file");
     CmdArgs.push_back(DepFile);
 
     // Add a default target if one wasn't specified.
     if (!Args.hasArg(options::OPT_MT) && !Args.hasArg(options::OPT_MQ)) {
       const char *DepTarget;
 
       // If user provided -o, that is the dependency target, except
       // when we are only generating a dependency file.
void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
   // OBJCPLUS_INCLUDE_PATH - system includes enabled when compiling ObjC++.
   addDirectoryList(Args, CmdArgs, "-objcxx-isystem", "OBJCPLUS_INCLUDE_PATH");
 
   // While adding the include arguments, we also attempt to retrieve the
   // arguments of related offloading toolchains or arguments that are specific
   // of an offloading programming model.
 
   // Add C++ include arguments, if needed.
-  if (types::isCXX(Inputs[0].getType())) {
-    getToolChain().AddClangCXXStdlibIncludeArgs(Args, CmdArgs);
-    addExtraOffloadCXXStdlibIncludeArgs(C, JA, Args, CmdArgs);
-  }
+  if (types::isCXX(Inputs[0].getType()))
+    forAllAssociatedToolChains(C, JA, getToolChain(),
+                               [&Args, &CmdArgs](const ToolChain &TC) {
+                                 TC.AddClangCXXStdlibIncludeArgs(Args, CmdArgs);
+                               });
 
   // Add system include arguments for all targets but IAMCU.
-  if (!IsIAMCU) {
-    getToolChain().AddClangSystemIncludeArgs(Args, CmdArgs);
-    addExtraOffloadClangSystemIncludeArgs(C, JA, Args, CmdArgs);
-  } else {
+  if (!IsIAMCU)
+    forAllAssociatedToolChains(C, JA, getToolChain(),
+                               [&Args, &CmdArgs](const ToolChain &TC) {
+                                 TC.AddClangSystemIncludeArgs(Args, CmdArgs);
+                               });
+  else {
     // For IAMCU add special include arguments.
     getToolChain().AddIAMCUIncludeArgs(Args, CmdArgs);
   }
 
-  // Add offload include arguments, if needed.
-  addExtraOffloadSpecificIncludeArgs(C, JA, Args, CmdArgs);
+  // Add offload include arguments specific for CUDA if that is required.
+  if (JA.isOffloading(Action::OFK_Cuda))
+    getToolChain().AddCudaIncludeArgs(Args, CmdArgs);
 }
 
 // FIXME: Move to target hook.
 static bool isSignedCharDefault(const llvm::Triple &Triple) {
   switch (Triple.getArch()) {
   default:
     return true;
 

test/Driver/cuda-version-check.cu

 // RUN:    --sysroot=%S/Inputs/CUDA 2>&1 %s | \
 // RUN:    FileCheck %s --check-prefix=OK
 
 // --no-cuda-version-check should suppress all of these errors.
 // RUN: %clang -v -### --cuda-gpu-arch=sm_60 --sysroot=%S/Inputs/CUDA 2>&1 \
 // RUN:    --no-cuda-version-check %s | \
 // RUN:    FileCheck %s --check-prefix=OK
 
+// We need to make sure the version check is done only for the device toolchain,
+// therefore we should not get an error in host-only mode. We use the -S here
+// to avoid the error being produced in case by the assembler tool, which does
+// the same check.
+// RUN: %clang -v -### --cuda-gpu-arch=sm_60 --cuda-host-only --sysroot=%S/Inputs/CUDA -S 2>&1 %s | \
+// RUN:    FileCheck %s --check-prefix=OK
+// RUN: %clang -v -### --cuda-gpu-arch=sm_60 --cuda-device-only --sysroot=%S/Inputs/CUDA -S 2>&1 %s | \
+// RUN:    FileCheck %s --check-prefix=ERR_SM60
+
 // OK-NOT: error: GPU arch
 
 // OK_SM35-NOT: error: GPU arch sm_35
 
 // We should only get one error per architecture.
 // ERR_SM60: error: GPU arch sm_60 {{.*}}
 // ERR_SM60-NOT: error: GPU arch sm_60
 
 // ERR_SM61: error: GPU arch sm_61 {{.*}}
 // ERR_SM61-NOT: error: GPU arch sm_61