[ThinLTO] Pass -save-temps to LTO backend for distributed ThinLTO builds

[tejohnson]

Summary

The clang driver option -save-temps was not passed to the LTO config,
so when invoking the ThinLTO backends via clang during distributed
builds there was no way to get LTO to save temp files.

Getting this to work with ThinLTO distributed builds also required
changing the driver to avoid a separate compile step to emit unoptimized
bitcode when the input was already bitcode under -save-temps. Not only is
this unnecessary in general, it is problematic for ThinLTO backends since
the temporary bitcode file to the backend would not match the module path
in the combined index, leading to incorrect ThinLTO backend index-based
optimizations.

[tejohnson]

ping

[pcc]

include/clang/Driver/Options.td

2255 ↗

(On Diff #140832)

This is still just a DriverOption, right?

lib/Frontend/CompilerInvocation.cpp

747 ↗

(On Diff #140832)

Why do you need the "./" part?

test/CodeGen/thinlto_backend.ll

30 ↗

(On Diff #140832)

Should there be another test for -save-temps=cwd?

[tejohnson]

include/clang/Driver/Options.td

2255 ↗

(On Diff #140832)

No, we need to pass to cc1 to get it into the CodeGenOptions. See the changes to Clang.cpp (passes to cc1 invocation) and CompilerInvocation.cpp (consumes during cc1 invocation).

lib/Frontend/CompilerInvocation.cpp

747 ↗

(On Diff #140832)

Not needed, removed.

test/CodeGen/thinlto_backend.ll

30 ↗

(On Diff #140832)

Added -save-temps=cwd and -save-temps

[tejohnson]

Addressed comments

[pcc]

include/clang/Driver/Options.td

2255 ↗

(On Diff #140832)

You are producing a -save-temps= flag, not -save-temps.

i.e. I think you should be able to remove the CC1Option from this line and keep it on line 2253.

test/CodeGen/thinlto_backend.ll

30 ↗

(On Diff #140832)

This isn't actually testing that the behaviour is different between -save-temps=obj and -save-temps=cwd, right?

Maybe create a new directory, cd there and verify that the files are created there?

[tejohnson]

include/clang/Driver/Options.td

2255 ↗

(On Diff #140832)

Oh, I see what you are saying. Removed CC1Option from the plain -save-temps.

test/CodeGen/thinlto_backend.ll

30 ↗

(On Diff #140832)

Using different directories for all 3 cases now.

[tejohnson]

Address comments

[pcc]

LGTM

[bjope]

cfe/trunk/lib/Driver/ToolChains/Clang.cpp

3270

A is not used, so this does not compile when using -Werror:

../tools/clang/lib/Driver/ToolChains/Clang.cpp:3270:18: error: unused variable 'A' [-Werror,-Wunused-variable]
  if (const Arg *A = Args.getLastArg(options::OPT_save_temps_EQ))

[tejohnson]

cfe/trunk/lib/Driver/ToolChains/Clang.cpp

3270

Sorry, will fix shortly.

[tejohnson]

cfe/trunk/lib/Driver/ToolChains/Clang.cpp

3270

Fixed in r330210

cfe/trunk/include/clang/Driver/Options.td

 def rtlib_EQ : Joined<["-", "--"], "rtlib=">,
   HelpText<"Compiler runtime library to use">;
 def frtlib_add_rpath: Flag<["-"], "frtlib-add-rpath">, Flags<[NoArgumentUnused]>,
   HelpText<"Add -rpath with architecture-specific resource directory to the linker flags">;
 def fno_rtlib_add_rpath: Flag<["-"], "fno-rtlib-add-rpath">, Flags<[NoArgumentUnused]>,
   HelpText<"Do not add -rpath with architecture-specific resource directory to the linker flags">;
 def r : Flag<["-"], "r">, Flags<[LinkerInput,NoArgumentUnused]>,
         Group<Link_Group>;
-def save_temps_EQ : Joined<["-", "--"], "save-temps=">, Flags<[DriverOption]>,
+def save_temps_EQ : Joined<["-", "--"], "save-temps=">, Flags<[CC1Option, DriverOption]>,
   HelpText<"Save intermediate compilation results.">;
 def save_temps : Flag<["-", "--"], "save-temps">, Flags<[DriverOption]>,
   Alias<save_temps_EQ>, AliasArgs<["cwd"]>,
   HelpText<"Save intermediate compilation results">;
 def save_stats_EQ : Joined<["-", "--"], "save-stats=">, Flags<[DriverOption]>,
   HelpText<"Save llvm statistics.">;
 def save_stats : Flag<["-", "--"], "save-stats">, Flags<[DriverOption]>,
   Alias<save_stats_EQ>, AliasArgs<["cwd"]>,

cfe/trunk/include/clang/Frontend/CodeGenOptions.h

public:
   /// importing.
   std::string ThinLTOIndexFile;
 
   /// Name of a file that can optionally be written with minimized bitcode
   /// to be used as input for the ThinLTO thin link step, which only needs
   /// the summary and module symbol table (and not, e.g. any debug metadata).
   std::string ThinLinkBitcodeFile;
 
+  /// Prefix to use for -save-temps output.
+  std::string SaveTempsFilePrefix;
+
   /// Name of file passed with -fcuda-include-gpubinary option to forward to
   /// CUDA runtime back-end for incorporating them into host-side object file.
   std::string CudaGpuBinaryFileName;
 
   /// The name of the file to which the backend should save YAML optimization
   /// records.
   std::string OptRecordFile;
 

cfe/trunk/lib/CodeGen/BackendUtil.cpp

for (auto &I : ImportList) {
     ModuleMap.insert({I.first(), *BMOrErr});
 
     OwnedImports.push_back(std::move(*MBOrErr));
   }
   auto AddStream = [&](size_t Task) {
     return llvm::make_unique<lto::NativeObjectStream>(std::move(OS));
   };
   lto::Config Conf;
+  if (CGOpts.SaveTempsFilePrefix != "") {
+    if (Error E = Conf.addSaveTemps(CGOpts.SaveTempsFilePrefix + ".",
+                                    /* UseInputModulePath */ false)) {
+      handleAllErrors(std::move(E), [&](ErrorInfoBase &EIB) {
+        errs() << "Error setting up ThinLTO save-temps: " << EIB.message()
+               << '\n';
+      });
+    }
+  }
   Conf.CPU = TOpts.CPU;
   Conf.CodeModel = getCodeModel(CGOpts);
   Conf.MAttrs = TOpts.Features;
   Conf.RelocModel = CGOpts.RelocationModel;
   Conf.CGOptLevel = getCGOptLevel(CGOpts);
   initTargetOptions(Conf.Options, CGOpts, TOpts, LOpts, HeaderOpts);
   Conf.SampleProfile = std::move(SampleProfile);
   Conf.UseNewPM = CGOpts.ExperimentalNewPassManager;

cfe/trunk/lib/Driver/Driver.cpp

const Tool *combineAssembleBackend(ArrayRef<JobActionInfo> ActionInfo,
     Inputs = &BJ->getInputs();
     AppendCollapsedOffloadAction(CollapsedOffloadAction, ActionInfo,
                                  /*NumElements=*/2);
     return T;
   }
   const Tool *combineBackendCompile(ArrayRef<JobActionInfo> ActionInfo,
                                     const ActionList *&Inputs,
                                     ActionList &CollapsedOffloadAction) {
-    if (ActionInfo.size() < 2 || !canCollapsePreprocessorAction())
+    if (ActionInfo.size() < 2)
       return nullptr;
     auto *BJ = dyn_cast<BackendJobAction>(ActionInfo[0].JA);
     auto *CJ = dyn_cast<CompileJobAction>(ActionInfo[1].JA);
     if (!BJ || !CJ)
       return nullptr;
 
+    // Check if the initial input (to the compile job or its predessor if one
+    // exists) is LLVM bitcode. In that case, no preprocessor step is required
+    // and we can still collapse the compile and backend jobs when we have
+    // -save-temps. I.e. there is no need for a separate compile job just to
+    // emit unoptimized bitcode.
+    bool InputIsBitcode = true;
+    for (size_t i = 1; i < ActionInfo.size(); i++)
+      if (ActionInfo[i].JA->getType() != types::TY_LLVM_BC &&
+          ActionInfo[i].JA->getType() != types::TY_LTO_BC) {
+        InputIsBitcode = false;
+        break;
+      }
+    if (!InputIsBitcode && !canCollapsePreprocessorAction())
+      return nullptr;
+
     // Get compiler tool.
     const Tool *T = TC.SelectTool(*CJ);
     if (!T)
       return nullptr;
 
-    if (T->canEmitIR() && (SaveTemps || EmbedBitcode))
+    if (T->canEmitIR() && ((SaveTemps && !InputIsBitcode) || EmbedBitcode))
       return nullptr;
 
     Inputs = &CJ->getInputs();
     AppendCollapsedOffloadAction(CollapsedOffloadAction, ActionInfo,
                                  /*NumElements=*/2);
     return T;
   }
 

cfe/trunk/lib/Driver/ToolChains/Clang.cpp

void Clang::ConstructJob(Compilation &C, const JobAction &JA,
 
   if (const Arg *A = Args.getLastArg(options::OPT_fthinlto_index_EQ)) {
     if (!types::isLLVMIR(Input.getType()))
       D.Diag(diag::err_drv_argument_only_allowed_with) << A->getAsString(Args)
                                                        << "-x ir";
     Args.AddLastArg(CmdArgs, options::OPT_fthinlto_index_EQ);
   }
 
+  if (const Arg *A = Args.getLastArg(options::OPT_save_temps_EQ))
+    Args.AddLastArg(CmdArgs, options::OPT_save_temps_EQ);
+
   // Embed-bitcode option.
   if (C.getDriver().embedBitcodeInObject() && !C.getDriver().isUsingLTO() &&
       (isa<BackendJobAction>(JA) || isa<AssembleJobAction>(JA))) {
     // Add flags implied by -fembed-bitcode.
     Args.AddLastArg(CmdArgs, options::OPT_fembed_bitcode_EQ);
     // Disable all llvm IR level optimizations.
     CmdArgs.push_back("-disable-llvm-passes");
   }

cfe/trunk/lib/Frontend/CompilerInvocation.cpp

static void setPGOUseInstrumentor(CodeGenOptions &Opts,
   if (PGOReader->isIRLevelProfile())
     Opts.setProfileUse(CodeGenOptions::ProfileIRInstr);
   else
     Opts.setProfileUse(CodeGenOptions::ProfileClangInstr);
 }
 
 static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
                              DiagnosticsEngine &Diags,
-                             const TargetOptions &TargetOpts) {
+                             const TargetOptions &TargetOpts,
+                             const FrontendOptions &FrontendOpts) {
   bool Success = true;
   llvm::Triple Triple = llvm::Triple(TargetOpts.Triple);
 
   unsigned OptimizationLevel = getOptimizationLevel(Args, IK, Diags);
   // TODO: This could be done in Driver
   unsigned MaxOptLevel = 3;
   if (OptimizationLevel > MaxOptLevel) {
     // If the optimization level is not supported, fall back on the default
static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   }
   Opts.LTOUnit = Args.hasFlag(OPT_flto_unit, OPT_fno_lto_unit, false);
   if (Arg *A = Args.getLastArg(OPT_fthinlto_index_EQ)) {
     if (IK.getLanguage() != InputKind::LLVM_IR)
       Diags.Report(diag::err_drv_argument_only_allowed_with)
           << A->getAsString(Args) << "-x ir";
     Opts.ThinLTOIndexFile = Args.getLastArgValue(OPT_fthinlto_index_EQ);
   }
+  if (Arg *A = Args.getLastArg(OPT_save_temps_EQ))
+    Opts.SaveTempsFilePrefix =
+        llvm::StringSwitch<std::string>(A->getValue())
+            .Case("obj", FrontendOpts.OutputFile)
+            .Default(llvm::sys::path::filename(FrontendOpts.OutputFile).str());
+
   Opts.ThinLinkBitcodeFile = Args.getLastArgValue(OPT_fthin_link_bitcode_EQ);
 
   Opts.MSVolatile = Args.hasArg(OPT_fms_volatile);
 
   Opts.VectorizeLoop = Args.hasArg(OPT_vectorize_loops);
   Opts.VectorizeSLP = Args.hasArg(OPT_vectorize_slp);
 
   Opts.PreferVectorWidth = Args.getLastArgValue(OPT_mprefer_vector_width_EQ);
Success &=
                           false /*DefaultDiagColor*/, false /*DefaultShowOpt*/);
   ParseCommentArgs(LangOpts.CommentOpts, Args);
   ParseFileSystemArgs(Res.getFileSystemOpts(), Args);
   // FIXME: We shouldn't have to pass the DashX option around here
   InputKind DashX = ParseFrontendArgs(Res.getFrontendOpts(), Args, Diags,
                                       LangOpts.IsHeaderFile);
   ParseTargetArgs(Res.getTargetOpts(), Args, Diags);
   Success &= ParseCodeGenArgs(Res.getCodeGenOpts(), Args, DashX, Diags,
-                              Res.getTargetOpts());
+                              Res.getTargetOpts(), Res.getFrontendOpts());
   ParseHeaderSearchArgs(Res.getHeaderSearchOpts(), Args,
                         Res.getFileSystemOpts().WorkingDir);
   if (DashX.getFormat() == InputKind::Precompiled ||
       DashX.getLanguage() == InputKind::LLVM_IR) {
     // ObjCAAutoRefCount and Sanitize LangOpts are used to setup the
     // PassManager in BackendUtil.cpp. They need to be initializd no matter
     // what the input type is.
     if (Args.hasArg(OPT_fobjc_arc))

cfe/trunk/test/CodeGen/thinlto_backend.ll

 ; CHECK-OBJ-IGNORE-EMPTY: U f2
 
 ; Ensure we don't fail with index and non-ThinLTO object file, and output must
 ; be empty file.
 ; RUN: opt -o %t5.o %s
 ; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t4.o -x ir %t5.o -c -fthinlto-index=%t.thinlto.bc
 ; RUN: llvm-nm %t4.o | count 0
 
-; Ensure f2 was imported
-; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t3.o -x ir %t1.o -c -fthinlto-index=%t.thinlto.bc
+; Ensure f2 was imported. Check for all 3 flavors of -save-temps[=cwd|obj].
+; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t3.o -x ir %t1.o -c -fthinlto-index=%t.thinlto.bc -save-temps=obj
+; RUN: llvm-dis %t1.s.0.3.import.bc -o - | FileCheck --check-prefix=CHECK-IMPORT %s
+; RUN: mkdir -p %T/dir1
+; RUN: (cd %T/dir1 && %clang -target x86_64-unknown-linux-gnu -O2 -o %t3.o -x ir %t1.o -c -fthinlto-index=%t.thinlto.bc -save-temps=cwd)
+; RUN: llvm-dis %T/dir1/*1.s.0.3.import.bc -o - | FileCheck --check-prefix=CHECK-IMPORT %s
+; RUN: mkdir -p %T/dir2
+; RUN: (cd %T/dir2 && %clang -target x86_64-unknown-linux-gnu -O2 -o %t3.o -x ir %t1.o -c -fthinlto-index=%t.thinlto.bc -save-temps)
+; RUN: llvm-dis %T/dir2/*1.s.0.3.import.bc -o - | FileCheck --check-prefix=CHECK-IMPORT %s
+; CHECK-IMPORT: define available_externally void @f2()
 ; RUN: llvm-nm %t3.o | FileCheck --check-prefix=CHECK-OBJ %s
 ; CHECK-OBJ: T f1
 ; CHECK-OBJ-NOT: U f2
 
 ; Ensure we get expected error for input files without summaries
 ; RUN: opt -o %t2.o %s
 ; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t3.o -x ir %t1.o -c -fthinlto-index=%t.thinlto.bc 2>&1 | FileCheck %s -check-prefix=CHECK-ERROR2
 ; CHECK-ERROR2: Error loading imported file '{{.*}}': Could not find module summary

cfe/trunk/test/Driver/thinlto_backend.c

 // RUN: %clang -O2 %s -flto=thin -c -o %t.o
 // RUN: llvm-lto -thinlto -o %t %t.o
 
 // -fthinlto_index should be passed to cc1
 // RUN: %clang -O2 -o %t1.o -x ir %t.o -c -fthinlto-index=%t.thinlto.bc -### 2>&1 | FileCheck %s -check-prefix=CHECK-THINLTOBE-ACTION
 // CHECK-THINLTOBE-ACTION: -fthinlto-index=
 
+// -save-temps should be passed to cc1
+// RUN: %clang -O2 -o %t1.o -x ir %t.o -c -fthinlto-index=%t.thinlto.bc -save-temps -### 2>&1 | FileCheck %s -check-prefix=CHECK-SAVE-TEMPS -check-prefix=CHECK-SAVE-TEMPS-CWD
+// RUN: %clang -O2 -o %t1.o -x ir %t.o -c -fthinlto-index=%t.thinlto.bc -save-temps=cwd -### 2>&1 | FileCheck %s -check-prefix=CHECK-SAVE-TEMPS -check-prefix=CHECK-SAVE-TEMPS-CWD
+// RUN: %clang -O2 -o %t1.o -x ir %t.o -c -fthinlto-index=%t.thinlto.bc -save-temps=obj -### 2>&1 | FileCheck %s -check-prefix=CHECK-SAVE-TEMPS -check-prefix=CHECK-SAVE-TEMPS-OBJ
+// CHECK-SAVE-TEMPS-NOT: -emit-llvm-bc
+// CHECK-SAVE-TEMPS-CWD: -save-temps=cwd
+// CHECK-SAVE-TEMPS-OBJ: -save-temps=obj
+// CHECK-SAVE-TEMPS-NOT: -emit-llvm-bc
+
 // Ensure clang driver gives the expected error for incorrect input type
 // RUN: not %clang -O2 -o %t1.o %s -c -fthinlto-index=%t.thinlto.bc 2>&1 | FileCheck %s -check-prefix=CHECK-WARNING
 // CHECK-WARNING: error: invalid argument '-fthinlto-index={{.*}}' only allowed with '-x ir'