[Clang] Fix the new driver crashing when using '-fsyntax-only'

[jhuber6]

Summary

The new driver currently crashses when attempting to use the
'-fsyntax-only' option. This is because the option causes all output to
be given the `TY_Nothing' type which should signal the end of the
pipeline. The new driver was not treating this correctly and attempting
to use empty input. This patch fixes the handling so we do not attempt
to continue when the input is nothing.

One concession is that we must now check when generating the arguments
for Clang if the input is of 'TY_Nothing'. This is because the new
driver will only create code if the device code is a dependency on the
host, creating the output without the dependency would require a
complete rewrite of the logic as we do not maintain any state between
calls to 'BuildOffloadingActions' so I believe this is the most
straightforward method.

[tra]

Does this patch obviate D133133 or is it purely for the new driver functionality?

[jhuber6]

In D133161#3765469, @tra wrote:

Does this patch obviate D133133 or is it purely for the new driver functionality?

If the intent of that patch was to prevent it from emitting an error with -o when -fsyntax-only is used then this is not a replacement. This is just a new driver specific change to the logic that correctly handles TY_Nothing output.

[tra]

OK. I'm going to land it then.

clang/lib/Driver/Driver.cpp

4388–4390

any_of(A->getType() == types::TY_Nothing) looks like a rather indirect way to infer that we have -fsyntax-only. Would there be any other legitimate case when we'd have Ty_Nothing actions?

4395

Assuming that we're guaranteed that all actions produce no outputs, can we just pass an empty array if -fsyntax-only is specified?

clang/lib/Driver/ToolChains/Clang.cpp

4499–4500

.. and if we don't propagate no-output actions, there would be no need to filter them here.

[jhuber6]

clang/lib/Driver/Driver.cpp

4388–4390

This is more or less how the existing pipeline handles it. We shouldn't propagate TY_Nothing attributes regardless, -fsyntax-only just happens to create them. So this is more correct I believe.

4395

No, this is required to make the device actions be generated. The old driver manually added device actions to the final action list, while the new driver adds them as host dependencies. I prefer the new method as it requires no internal state so it can just be a simple function like here.

clang/lib/Driver/ToolChains/Clang.cpp

4499–4500

Without that we would need to change the logic to append to the final action list directly, which would be an enormous hack given the current architecture of the new driver. FWIW I think this is a valid operation regardless, this didn't have a good failure mode in the first place, it just crashed on some assertion later if you did it. This at least makes sure that we'll get some "no input" error message AFAIK.

[tra]

clang/lib/Driver/Driver.cpp

4388–4390

OK. Would it make sense to add a helper method with descriptive name? E.g. hasValidOutput so future readers do not have to guess what we're doing and why.

clang/lib/Driver/Driver.cpp

for (Action::OffloadKind Kind : OffloadKinds) {
     for (phases::ID Phase : PL) {
       if (Phase == phases::Link) {
         assert(Phase == PL.back() && "linking must be final compilation step.");
         break;
       }
 
       auto TCAndArch = TCAndArchs.begin();
       for (Action *&A : DeviceActions) {
+        if (A->getType() == types::TY_Nothing)
+          continue;
+
         A = ConstructPhaseAction(C, Args, Phase, A, Kind);
 
         if (isa<CompileJobAction>(A) && isa<CompileJobAction>(HostAction) &&
-            Kind == Action::OFK_OpenMP) {
+            Kind == Action::OFK_OpenMP &&
+            HostAction->getType() != types::TY_Nothing) {
           // OpenMP offloading has a dependency on the host compile action to
           // identify which declarations need to be emitted. This shouldn't be
           // collapsed with any other actions so we can use it in the device.
           HostAction->setCannotBeCollapsedWithNextDependentAction();
           OffloadAction::HostDependence HDep(
               *HostAction, *C.getSingleOffloadToolChain<Action::OFK_Host>(),
               TCAndArch->second.data(), Kind);
           OffloadAction::DeviceDependences DDep;
if (C.isOffloadingHostKind(Action::OFK_Cuda) &&
     // Package all the offloading actions into a single output that can be
     // embedded in the host and linked.
     Action *PackagerAction =
         C.MakeAction<OffloadPackagerJobAction>(OffloadActions, types::TY_Image);
     DDep.add(*PackagerAction, *C.getSingleOffloadToolChain<Action::OFK_Host>(),
              nullptr, Action::OFK_None);
   }
 
+  // If we are unable to embed a single device output into the host, we need to
+  // add each device output as a host dependency to ensure they are still built.
+  bool SingleDeviceOutput = !llvm::any_of(OffloadActions, [](Action *A) {
+    return A->getType() == types::TY_Nothing;
+  }) && isa<CompileJobAction>(HostAction);
   OffloadAction::HostDependence HDep(
       *HostAction, *C.getSingleOffloadToolChain<Action::OFK_Host>(),
       /*BoundArch=*/nullptr, isa<CompileJobAction>(HostAction) ? DDep : DDeps);
-  return C.MakeAction<OffloadAction>(
-      HDep, isa<CompileJobAction>(HostAction) ? DDep : DDeps);
+  return C.MakeAction<OffloadAction>(HDep, SingleDeviceOutput ? DDep : DDeps);
 }
 
 Action *Driver::ConstructPhaseAction(
     Compilation &C, const ArgList &Args, phases::ID Phase, Action *Input,
     Action::OffloadKind TargetDeviceOffloadKind) const {
   llvm::PrettyStackTraceString CrashInfo("Constructing phase actions");
 
   // Some types skip the assembler phase (e.g., llvm-bc), but we can't

clang/lib/Driver/ToolChains/Clang.cpp

void Clang::ConstructJob(Compilation &C, const JobAction &JA,
   }();
 
   InputInfoList ModuleHeaderInputs;
   InputInfoList ExtractAPIInputs;
   InputInfoList HostOffloadingInputs;
   const InputInfo *CudaDeviceInput = nullptr;
   const InputInfo *OpenMPDeviceInput = nullptr;
   for (const InputInfo &I : Inputs) {
-    if (&I == &Input) {
-      // This is the primary input.
+    if (&I == &Input || I.getType() == types::TY_Nothing) {
+      // This is the primary input or contains nothing.
     } else if (IsHeaderModulePrecompile &&
                types::getPrecompiledType(I.getType()) == types::TY_PCH) {
       types::ID Expected = HeaderModuleInput.getType();
       if (I.getType() != Expected) {
         D.Diag(diag::err_drv_module_header_wrong_kind)
             << I.getFilename() << types::getTypeName(I.getType())
             << types::getTypeName(Expected);
       }

clang/test/Driver/cuda-bindings.cu

 // MULTI-D-ONLY-NEXT: # "nvptx64-nvidia-cuda" - "NVPTX::Assembler", inputs: ["[[PTX_70]]"], output: "[[CUBIN_70:.+]]"
 // MULTI-D-ONLY-NEXT: # "nvptx64-nvidia-cuda" - "clang", inputs: ["[[INPUT]]"], output: "[[PTX_52:.+]]"
 // MULTI-D-ONLY-NEXT: # "nvptx64-nvidia-cuda" - "NVPTX::Assembler", inputs: ["[[PTX_52]]"], output: "[[CUBIN_52:.+]]"
 //
 // RUN: %clang -### -target powerpc64le-ibm-linux-gnu --offload-new-driver -ccc-print-bindings \
 // RUN:        --offload-arch=sm_70 --offload-arch=sm_52 --offload-device-only -c -o %t %s 2>&1 \
 // RUN: | FileCheck -check-prefix=MULTI-D-ONLY-O %s
 // MULTI-D-ONLY-O: error: cannot specify -o when generating multiple output files
+
+//
+// Check to ensure that we can use '-fsyntax-only' for CUDA output with the new
+// driver.
+// 
+// RUN: %clang -### -target powerpc64le-ibm-linux-gnu --offload-new-driver \
+// RUN:        -fsyntax-only --offload-arch=sm_70 --offload-arch=sm_52 -c %s 2>&1 \
+// RUN: | FileCheck -check-prefix=SYNTAX-ONLY %s
+// SYNTAX-ONLY: "-cc1" "-triple" "nvptx64-nvidia-cuda"{{.*}}"-fsyntax-only"
+// SYNTAX-ONLY: "-cc1" "-triple" "nvptx64-nvidia-cuda"{{.*}}"-fsyntax-only"
+// SYNTAX-ONLY: "-cc1" "-triple" "powerpc64le-ibm-linux-gnu"{{.*}}"-fsyntax-only"

clang/test/Driver/hip-binding.hip

 // MULTI-D-ONLY-NEXT: # "amdgcn-amd-amdhsa" - "AMDGCN::Linker", inputs: ["[[GFX908]]"], output: "[[GFX908_OUT:.+]]"
 // MULTI-D-ONLY-NEXT: # "amdgcn-amd-amdhsa" - "clang", inputs: ["[[INPUT]]"], output: "[[GFX90a:.+]]"
 // MULTI-D-ONLY-NEXT: # "amdgcn-amd-amdhsa" - "AMDGCN::Linker", inputs: ["[[GFX90a]]"], output: "[[GFX90a_OUT:.+]]"
 //
 // RUN: %clang -### --target=x86_64-linux-gnu --offload-new-driver -ccc-print-bindings \
 // RUN:        --offload-arch=gfx90a --offload-arch=gfx908 --offload-device-only -c -o %t %s 2>&1 \
 // RUN: | FileCheck -check-prefix=MULTI-D-ONLY-O %s
 // MULTI-D-ONLY-O: error: cannot specify -o when generating multiple output files
+
+//
+// Check to ensure that we can use '-fsyntax-only' for HIP output with the new
+// driver.
+// 
+// RUN: %clang -### -target x86_64-unknown-linux-gnu --offload-new-driver \
+// RUN:        -fsyntax-only --offload-arch=gfx90a --offload-arch=gfx908 -c %s 2>&1 \
+// RUN: | FileCheck -check-prefix=SYNTAX-ONLY %s
+// SYNTAX-ONLY: "-cc1" "-triple" "amdgcn-amd-amdhsa"{{.*}}"-fsyntax-only"
+// SYNTAX-ONLY: "-cc1" "-triple" "amdgcn-amd-amdhsa"{{.*}}"-fsyntax-only"
+// SYNTAX-ONLY: "-cc1" "-triple" "x86_64-unknown-linux-gnu"{{.*}}"-fsyntax-only"

clang/test/Driver/openmp-offload.c

 
 // CHK-NEW-DRIVER-DEBUG: clang-linker-wrapper{{.*}} "--device-debug"
 
 /// Check arguments to the linker wrapper
 // RUN:   %clang -### --target=powerpc64le-linux -fopenmp=libomp -fopenmp-targets=powerpc64le-ibm-linux-gnu \
 // RUN:     -mllvm -abc %s 2>&1 | FileCheck -check-prefix=CHK-NEW-DRIVER-MLLVM %s
 
 // CHK-NEW-DRIVER-MLLVM: clang-linker-wrapper{{.*}} "-abc"
+
+//
+// Ensure that we generate the correct bindings for '-fsyntax-only' for OpenMP.
+//
+// RUN:   %clang -### --target=powerpc64le-linux -fopenmp=libomp -fopenmp-targets=powerpc64le-ibm-linux-gnu \
+// RUN:     -fsyntax-only -ccc-print-bindings %s 2>&1 | FileCheck -check-prefix=CHK-SYNTAX-ONLY %s
+// CHK-SYNTAX-ONLY: # "powerpc64le-ibm-linux-gnu" - "clang", inputs: ["[[INPUT:.+]]"], output: (nothing)
+// CHK-SYNTAX-ONLY: # "powerpc64le-unknown-linux" - "clang", inputs: ["[[INPUT]]", (nothing)], output: (nothing)
+
+//
+// Ensure that we can generate the correct arguments for '-fsyntax-only' for
+// OpenMP.
+//
+// RUN:   %clang -### --target=powerpc64le-linux -fopenmp=libomp -fopenmp-targets=powerpc64le-ibm-linux-gnu \
+// RUN:     -fsyntax-only %s 2>&1 | FileCheck -check-prefix=CHK-SYNTAX-ONLY-ARGS %s
+// CHK-SYNTAX-ONLY-ARGS: "-cc1" "-triple" "powerpc64le-ibm-linux-gnu"{{.*}}"-fsyntax-only"
+// CHK-SYNTAX-ONLY-ARGS: "-cc1" "-triple" "powerpc64le-unknown-linux"{{.*}}"-fsyntax-only"