Refactor `initTargetOptions` out of `EmitAssemblyHelper::CreateTargetMachine` and use it to initialize TargetOptions for ThinLTO Backends

[tejohnson]

I think you won't get the correct handling of -emit-llvm and -emit-llvm-bc since we don't get the handling for Backend_Emit* in EmitAssemblyHelper::EmitAssembly.

clang/lib/CodeGen/BackendUtil.cpp

595

What about this - it isn't used in the TargetOptions. There are a few other things passed directly to createTargetMachine below. Will -fpic get handled correctly, e.g.? Ah, looks like lto::Config has fields for that and the code model etc, that are essentially getting the default values on this path (when used via clang - that will need to be set up in lto::Config in runThinLTOBackend as well.

982

Why did this move?

[mehdi_amini]

clang/lib/CodeGen/BackendUtil.cpp

982

"History": it is useless now. I first refactored the initTargetOptions as a member on EmitAssemblyHelper rather than a static function.

[mehdi_amini]

In D31114#704649, @tejohnson wrote:

I think you won't get the correct handling of -emit-llvm and -emit-llvm-bc since we don't get the handling for Backend_Emit* in EmitAssemblyHelper::EmitAssembly.

I was not trying to achieve this. And your approach of an "OptimizeOnly" or "DisableCodeGen" on the lot::Config for this purpose makes sense.

[tejohnson]

In D31114#704726, @mehdi_amini wrote:

In D31114#704649, @tejohnson wrote:

I think you won't get the correct handling of -emit-llvm and -emit-llvm-bc since we don't get the handling for Backend_Emit* in EmitAssemblyHelper::EmitAssembly.

I was not trying to achieve this. And your approach of an "OptimizeOnly" or "DisableCodeGen" on the lot::Config for this purpose makes sense.

I'm confused - are you saying you now think D31100 and D31101 are the right approach?

Ah, I see that in thinBackend the created TM is used by handleAsmUndefinedRefs and opt(), so this patch is complementary. But in that case, to set up the TM accurately, this code does need to set up the other fields of lto::Config read by createTargetMachine, e.g. RelocModel, CodeModel, etc.

[mehdi_amini]

In D31114#704728, @tejohnson wrote:

In D31114#704726, @mehdi_amini wrote:

In D31114#704649, @tejohnson wrote:

I think you won't get the correct handling of -emit-llvm and -emit-llvm-bc since we don't get the handling for Backend_Emit* in EmitAssemblyHelper::EmitAssembly.

I was not trying to achieve this. And your approach of an "OptimizeOnly" or "DisableCodeGen" on the lot::Config for this purpose makes sense.

I'm confused - are you saying you now think D31100 and D31101 are the right approach?

I believe your patch is the right approach when clang needs to get the optimized IR, which is the case for -emit-llvm/-emit-bc. I believe that it shouldn't do that when it expects an object file.

Ah, I see that in thinBackend the created TM is used by handleAsmUndefinedRefs and opt(), so this patch is complementary. But in that case, to set up the TM accurately, this code does need to set up the other fields of lto::Config read by createTargetMachine, e.g. RelocModel, CodeModel, etc.

Right.
Forcing to use the LTO API for this ensures completeness: there isn't any target flag that clang could use that wouldn't be exposed through LTO.

[tejohnson]

In D31114#704733, @mehdi_amini wrote:

In D31114#704728, @tejohnson wrote:

In D31114#704726, @mehdi_amini wrote:

In D31114#704649, @tejohnson wrote:

I think you won't get the correct handling of -emit-llvm and -emit-llvm-bc since we don't get the handling for Backend_Emit* in EmitAssemblyHelper::EmitAssembly.

I was not trying to achieve this. And your approach of an "OptimizeOnly" or "DisableCodeGen" on the lot::Config for this purpose makes sense.

I'm confused - are you saying you now think D31100 and D31101 are the right approach?

I believe your patch is the right approach when clang needs to get the optimized IR, which is the case for -emit-llvm/-emit-bc. I believe that it shouldn't do that when it expects an object file.

What about for -S, which presumably maps to Backend_EmitAssembly? The LTO Config does have a CodeGenFileType field, which defaults to ObjectFile, but could be set here to AssemblyFile (see the handling in EmitAssemblyHelper::AddEmitPasses). I believe if you include the test case I added in D31101 which emits assembly that it will fail with this patch, for example. Since clang already has to handle codegen for all output types, I am not sure why it is better to have the LTO API handle the output for some of the cases and not others?

Ah, I see that in thinBackend the created TM is used by handleAsmUndefinedRefs and opt(), so this patch is complementary. But in that case, to set up the TM accurately, this code does need to set up the other fields of lto::Config read by createTargetMachine, e.g. RelocModel, CodeModel, etc.

Right.
Forcing to use the LTO API for this ensures completeness: there isn't any target flag that clang could use that wouldn't be exposed through LTO.

I think that is already the case, here we aren't changing what is exposed through LTO just how we invoke it from this path.

[mehdi_amini]

In D31114#704748, @tejohnson wrote:

In D31114#704733, @mehdi_amini wrote:

In D31114#704728, @tejohnson wrote:

In D31114#704726, @mehdi_amini wrote:

In D31114#704649, @tejohnson wrote:

I think you won't get the correct handling of -emit-llvm and -emit-llvm-bc since we don't get the handling for Backend_Emit* in EmitAssemblyHelper::EmitAssembly.

I was not trying to achieve this. And your approach of an "OptimizeOnly" or "DisableCodeGen" on the lot::Config for this purpose makes sense.

I'm confused - are you saying you now think D31100 and D31101 are the right approach?

I believe your patch is the right approach when clang needs to get the optimized IR, which is the case for -emit-llvm/-emit-bc. I believe that it shouldn't do that when it expects an object file.

What about for -S, which presumably maps to Backend_EmitAssembly?

Should be handled by LTO (we added it recently I think, at least to libLTO).

The LTO Config does have a CodeGenFileType field, which defaults to ObjectFile, but could be set here to AssemblyFile (see the handling in EmitAssemblyHelper::AddEmitPasses). I believe if you include the test case I added in D31101 which emits assembly that it will fail with this patch, for example. Since clang already has to handle codegen for all output types, I am not sure why it is better to have the LTO API handle the output for some of the cases and not others?

It is not better, and I don't suggest that. I believe all the cases should go through the LTO API preferably (or the LTO API should always use clang for codegen, but I don't believe anyone would consider that).

[tejohnson]

In D31114#704749, @mehdi_amini wrote:

In D31114#704748, @tejohnson wrote:

In D31114#704733, @mehdi_amini wrote:

In D31114#704728, @tejohnson wrote:

In D31114#704726, @mehdi_amini wrote:

In D31114#704649, @tejohnson wrote:

I think you won't get the correct handling of -emit-llvm and -emit-llvm-bc since we don't get the handling for Backend_Emit* in EmitAssemblyHelper::EmitAssembly.

I was not trying to achieve this. And your approach of an "OptimizeOnly" or "DisableCodeGen" on the lot::Config for this purpose makes sense.

I'm confused - are you saying you now think D31100 and D31101 are the right approach?

I believe your patch is the right approach when clang needs to get the optimized IR, which is the case for -emit-llvm/-emit-bc. I believe that it shouldn't do that when it expects an object file.

What about for -S, which presumably maps to Backend_EmitAssembly?

Should be handled by LTO (we added it recently I think, at least to libLTO).

The LTO Config does have a CodeGenFileType field, which defaults to ObjectFile, but could be set here to AssemblyFile (see the handling in EmitAssemblyHelper::AddEmitPasses). I believe if you include the test case I added in D31101 which emits assembly that it will fail with this patch, for example. Since clang already has to handle codegen for all output types, I am not sure why it is better to have the LTO API handle the output for some of the cases and not others?

It is not better, and I don't suggest that. I believe all the cases should go through the LTO API preferably (or the LTO API should always use clang for codegen, but I don't believe anyone would consider that).

I went ahead and updated my clang patch D31101 to only fall back to the clang handling in the -emit-llvm and -emit-llvm-bc cases.

I removed the test changes I had made to test/CodeGen/function-sections.c, you can include them here instead as a test for this fix.

[tejohnson]

Ping - I think this just needs a test in addition to undoing the AsmHelper definition move. I have some follow on changes I want to send that set up the rest of the lto::Config fields (e.g. the RelocModel etc).

[tejohnson]

As discussed with Mehdi offline, I am taking this one over. I just mailed D31508 which supersedes this one.

clang/lib/CodeGen/BackendUtil.cpp

static void addSymbolRewriterPass(const CodeGenOptions &Opts,
 
   llvm::SymbolRewriter::RewriteMapParser MapParser;
   for (const auto &MapFile : Opts.RewriteMapFiles)
     MapParser.parse(MapFile, &DL);
 
   MPM->add(createRewriteSymbolsPass(DL));
 }
 
+static void initTargetOptions(llvm::TargetOptions &Options,
+                              const CodeGenOptions &CodeGenOpts,
+                              const clang::TargetOptions &TargetOpts,
+                              const LangOptions &LangOpts,
+                              const HeaderSearchOptions &HSOpts
+
+                              ) {
+  Options.ThreadModel =
+      llvm::StringSwitch<llvm::ThreadModel::Model>(CodeGenOpts.ThreadModel)
+          .Case("posix", llvm::ThreadModel::POSIX)
+          .Case("single", llvm::ThreadModel::Single);
+
+  // Set float ABI type.
+  assert((CodeGenOpts.FloatABI == "soft" || CodeGenOpts.FloatABI == "softfp" ||
+          CodeGenOpts.FloatABI == "hard" || CodeGenOpts.FloatABI.empty()) &&
+         "Invalid Floating Point ABI!");
+  Options.FloatABIType =
+      llvm::StringSwitch<llvm::FloatABI::ABIType>(CodeGenOpts.FloatABI)
+          .Case("soft", llvm::FloatABI::Soft)
+          .Case("softfp", llvm::FloatABI::Soft)
+          .Case("hard", llvm::FloatABI::Hard)
+          .Default(llvm::FloatABI::Default);
+
+  // Set FP fusion mode.
+  switch (CodeGenOpts.getFPContractMode()) {
+  case CodeGenOptions::FPC_Off:
+    Options.AllowFPOpFusion = llvm::FPOpFusion::Strict;
+    break;
+  case CodeGenOptions::FPC_On:
+    Options.AllowFPOpFusion = llvm::FPOpFusion::Standard;
+    break;
+  case CodeGenOptions::FPC_Fast:
+    Options.AllowFPOpFusion = llvm::FPOpFusion::Fast;
+    break;
+  }
+
+  Options.UseInitArray = CodeGenOpts.UseInitArray;
+  Options.DisableIntegratedAS = CodeGenOpts.DisableIntegratedAS;
+  Options.CompressDebugSections = CodeGenOpts.CompressDebugSections;
+  Options.RelaxELFRelocations = CodeGenOpts.RelaxELFRelocations;
+
+  // Set EABI version.
+  Options.EABIVersion = llvm::StringSwitch<llvm::EABI>(TargetOpts.EABIVersion)
+                            .Case("4", llvm::EABI::EABI4)
+                            .Case("5", llvm::EABI::EABI5)
+                            .Case("gnu", llvm::EABI::GNU)
+                            .Default(llvm::EABI::Default);
+
+  if (LangOpts.SjLjExceptions)
+    Options.ExceptionModel = llvm::ExceptionHandling::SjLj;
+
+  Options.NoInfsFPMath = CodeGenOpts.NoInfsFPMath;
+  Options.NoNaNsFPMath = CodeGenOpts.NoNaNsFPMath;
+  Options.NoZerosInBSS = CodeGenOpts.NoZeroInitializedInBSS;
+  Options.UnsafeFPMath = CodeGenOpts.UnsafeFPMath;
+  Options.StackAlignmentOverride = CodeGenOpts.StackAlignment;
+  Options.FunctionSections = CodeGenOpts.FunctionSections;
+  Options.DataSections = CodeGenOpts.DataSections;
+  Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;
+  Options.EmulatedTLS = CodeGenOpts.EmulatedTLS;
+  Options.DebuggerTuning = CodeGenOpts.getDebuggerTuning();
+
+  Options.MCOptions.MCRelaxAll = CodeGenOpts.RelaxAll;
+  Options.MCOptions.MCSaveTempLabels = CodeGenOpts.SaveTempLabels;
+  Options.MCOptions.MCUseDwarfDirectory = !CodeGenOpts.NoDwarfDirectoryAsm;
+  Options.MCOptions.MCNoExecStack = CodeGenOpts.NoExecStack;
+  Options.MCOptions.MCIncrementalLinkerCompatible =
+      CodeGenOpts.IncrementalLinkerCompatible;
+  Options.MCOptions.MCPIECopyRelocations = CodeGenOpts.PIECopyRelocations;
+  Options.MCOptions.MCFatalWarnings = CodeGenOpts.FatalWarnings;
+  Options.MCOptions.AsmVerbose = CodeGenOpts.AsmVerbose;
+  Options.MCOptions.PreserveAsmComments = CodeGenOpts.PreserveAsmComments;
+  Options.MCOptions.ABIName = TargetOpts.ABI;
+  for (const auto &Entry : HSOpts.UserEntries)
+    if (!Entry.IsFramework &&
+        (Entry.Group == frontend::IncludeDirGroup::Quoted ||
+         Entry.Group == frontend::IncludeDirGroup::Angled ||
+         Entry.Group == frontend::IncludeDirGroup::System))
+      Options.MCOptions.IASSearchPaths.push_back(
+          Entry.IgnoreSysRoot ? Entry.Path : HSOpts.Sysroot + Entry.Path);
+}
+
 void EmitAssemblyHelper::CreatePasses(legacy::PassManager &MPM,
                                       legacy::FunctionPassManager &FPM) {
   // Handle disabling of all LLVM passes, where we want to preserve the
   // internal module before any optimization.
   if (CodeGenOpts.DisableLLVMPasses)
     return;
 
   PassManagerBuilderWrapper PMBuilder(CodeGenOpts, LangOpts);
void EmitAssemblyHelper::CreateTargetMachine(bool MustCreateTM) {
   assert(CodeModel != ~0u && "invalid code model!");
   llvm::CodeModel::Model CM = static_cast<llvm::CodeModel::Model>(CodeModel);
 
   std::string FeaturesStr =
       llvm::join(TargetOpts.Features.begin(), TargetOpts.Features.end(), ",");
 
   // Keep this synced with the equivalent code in tools/driver/cc1as_main.cpp.
   llvm::Optional<llvm::Reloc::Model> RM;
   RM = llvm::StringSwitch<llvm::Reloc::Model>(CodeGenOpts.RelocationModel)
            .Case("static", llvm::Reloc::Static)
            .Case("pic", llvm::Reloc::PIC_)
            .Case("ropi", llvm::Reloc::ROPI)
            .Case("rwpi", llvm::Reloc::RWPI)
            .Case("ropi-rwpi", llvm::Reloc::ROPI_RWPI)
            .Case("dynamic-no-pic", llvm::Reloc::DynamicNoPIC);
   assert(RM.hasValue() && "invalid PIC model!");
 
case 2:
     OptLevel = CodeGenOpt::Default;
     break; // O2/Os/Oz
   case 3:
     OptLevel = CodeGenOpt::Aggressive;
     break;
   }
 
   llvm::TargetOptions Options;
-
-  Options.ThreadModel =
-    llvm::StringSwitch<llvm::ThreadModel::Model>(CodeGenOpts.ThreadModel)
-      .Case("posix", llvm::ThreadModel::POSIX)
-      .Case("single", llvm::ThreadModel::Single);
-
-  // Set float ABI type.
-  assert((CodeGenOpts.FloatABI == "soft" || CodeGenOpts.FloatABI == "softfp" ||
-          CodeGenOpts.FloatABI == "hard" || CodeGenOpts.FloatABI.empty()) &&
-         "Invalid Floating Point ABI!");
-  Options.FloatABIType =
-      llvm::StringSwitch<llvm::FloatABI::ABIType>(CodeGenOpts.FloatABI)
-          .Case("soft", llvm::FloatABI::Soft)
-          .Case("softfp", llvm::FloatABI::Soft)
-          .Case("hard", llvm::FloatABI::Hard)
-          .Default(llvm::FloatABI::Default);
-
-  // Set FP fusion mode.
-  switch (CodeGenOpts.getFPContractMode()) {
-  case CodeGenOptions::FPC_Off:
-    Options.AllowFPOpFusion = llvm::FPOpFusion::Strict;
-    break;
-  case CodeGenOptions::FPC_On:
-    Options.AllowFPOpFusion = llvm::FPOpFusion::Standard;
-    break;
-  case CodeGenOptions::FPC_Fast:
-    Options.AllowFPOpFusion = llvm::FPOpFusion::Fast;
-    break;
-  }
-
-  Options.UseInitArray = CodeGenOpts.UseInitArray;
-  Options.DisableIntegratedAS = CodeGenOpts.DisableIntegratedAS;
-  Options.CompressDebugSections = CodeGenOpts.CompressDebugSections;
-  Options.RelaxELFRelocations = CodeGenOpts.RelaxELFRelocations;
-
-  // Set EABI version.
-  Options.EABIVersion = llvm::StringSwitch<llvm::EABI>(TargetOpts.EABIVersion)
-                            .Case("4", llvm::EABI::EABI4)
-                            .Case("5", llvm::EABI::EABI5)
-                            .Case("gnu", llvm::EABI::GNU)
-                            .Default(llvm::EABI::Default);
-
-  if (LangOpts.SjLjExceptions)
-    Options.ExceptionModel = llvm::ExceptionHandling::SjLj;
-
-  Options.NoInfsFPMath = CodeGenOpts.NoInfsFPMath;
-  Options.NoNaNsFPMath = CodeGenOpts.NoNaNsFPMath;
-  Options.NoZerosInBSS = CodeGenOpts.NoZeroInitializedInBSS;
-  Options.UnsafeFPMath = CodeGenOpts.UnsafeFPMath;
-  Options.StackAlignmentOverride = CodeGenOpts.StackAlignment;
-  Options.FunctionSections = CodeGenOpts.FunctionSections;
-  Options.DataSections = CodeGenOpts.DataSections;
-  Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;
-  Options.EmulatedTLS = CodeGenOpts.EmulatedTLS;
-  Options.DebuggerTuning = CodeGenOpts.getDebuggerTuning();
-
-  Options.MCOptions.MCRelaxAll = CodeGenOpts.RelaxAll;
-  Options.MCOptions.MCSaveTempLabels = CodeGenOpts.SaveTempLabels;
-  Options.MCOptions.MCUseDwarfDirectory = !CodeGenOpts.NoDwarfDirectoryAsm;
-  Options.MCOptions.MCNoExecStack = CodeGenOpts.NoExecStack;
-  Options.MCOptions.MCIncrementalLinkerCompatible =
-      CodeGenOpts.IncrementalLinkerCompatible;
-  Options.MCOptions.MCPIECopyRelocations = CodeGenOpts.PIECopyRelocations;
-  Options.MCOptions.MCFatalWarnings = CodeGenOpts.FatalWarnings;
-  Options.MCOptions.AsmVerbose = CodeGenOpts.AsmVerbose;
-  Options.MCOptions.PreserveAsmComments = CodeGenOpts.PreserveAsmComments;
-  Options.MCOptions.ABIName = TargetOpts.ABI;
-  for (const auto &Entry : HSOpts.UserEntries)
-    if (!Entry.IsFramework &&
-        (Entry.Group == frontend::IncludeDirGroup::Quoted ||
-         Entry.Group == frontend::IncludeDirGroup::Angled ||
-         Entry.Group == frontend::IncludeDirGroup::System))
-      Options.MCOptions.IASSearchPaths.push_back(
-          Entry.IgnoreSysRoot ? Entry.Path : HSOpts.Sysroot + Entry.Path);
-
+  initTargetOptions(Options, CodeGenOpts, TargetOpts, LangOpts, HSOpts);
   TM.reset(TheTarget->createTargetMachine(Triple, TargetOpts.CPU, FeaturesStr,
                                           Options, RM, CM, OptLevel));
 }
 
 bool EmitAssemblyHelper::AddEmitPasses(legacy::PassManager &CodeGenPasses,
                                        BackendAction Action,
                                        raw_pwrite_stream &OS) {
   // Add LibraryInfo.
if (HasSummary && *HasSummary)
       return BM;
   }
 
   return make_error<StringError>("Could not find module summary",
                                  inconvertibleErrorCode());
 }
 
 static void runThinLTOBackend(ModuleSummaryIndex *CombinedIndex, Module *M,
+                              const HeaderSearchOptions &HeaderOpts,
+                              const CodeGenOptions &CGOpts,
+                              const clang::TargetOptions &TOpts,
+                              const LangOptions &LOpts,
                               std::unique_ptr<raw_pwrite_stream> OS,
                               std::string SampleProfile) {
   StringMap<std::map<GlobalValue::GUID, GlobalValueSummary *>>
       ModuleToDefinedGVSummaries;
   CombinedIndex->collectDefinedGVSummariesPerModule(ModuleToDefinedGVSummaries);
 
   // We can simply import the values mentioned in the combined index, since
   // we should only invoke this using the individual indexes written out
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;
+  initTargetOptions(Conf.Options, CGOpts, TOpts, LOpts, HeaderOpts);
   Conf.SampleProfile = SampleProfile;
   if (Error E = thinBackend(
           Conf, 0, AddStream, *M, *CombinedIndex, ImportList,
           ModuleToDefinedGVSummaries[M->getModuleIdentifier()], ModuleMap)) {
     handleAllErrors(std::move(E), [&](ErrorInfoBase &EIB) {
       errs() << "Error running ThinLTO backend: " << EIB.message() << '\n';
     });
   }
 }
 
 void clang::EmitBackendOutput(DiagnosticsEngine &Diags,
                               const HeaderSearchOptions &HeaderOpts,
                               const CodeGenOptions &CGOpts,
                               const clang::TargetOptions &TOpts,
                               const LangOptions &LOpts,
                               const llvm::DataLayout &TDesc, Module *M,
                               BackendAction Action,
                               std::unique_ptr<raw_pwrite_stream> OS) {
+  EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
+
   if (!CGOpts.ThinLTOIndexFile.empty()) {
     // If we are performing a ThinLTO importing compile, load the function index
     // into memory and pass it into runThinLTOBackend, which will run the
     // function importer and invoke LTO passes.
     Expected<std::unique_ptr<ModuleSummaryIndex>> IndexOrErr =
         llvm::getModuleSummaryIndexForFile(CGOpts.ThinLTOIndexFile);
     if (!IndexOrErr) {
       logAllUnhandledErrors(IndexOrErr.takeError(), errs(),
                             "Error loading index file '" +
                             CGOpts.ThinLTOIndexFile + "': ");
       return;
     }
     std::unique_ptr<ModuleSummaryIndex> CombinedIndex = std::move(*IndexOrErr);
     // A null CombinedIndex means we should skip ThinLTO compilation
     // (LLVM will optionally ignore empty index files, returning null instead
     // of an error).
     bool DoThinLTOBackend = CombinedIndex != nullptr;
     if (DoThinLTOBackend) {
-      runThinLTOBackend(CombinedIndex.get(), M, std::move(OS),
-                        CGOpts.SampleProfileFile);
+      runThinLTOBackend(CombinedIndex.get(), M, HeaderOpts, CGOpts, TOpts,
+                        LOpts, std::move(OS), CGOpts.SampleProfileFile);
       return;
     }
   }
 
-  EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
-
   if (CGOpts.ExperimentalNewPassManager)
     AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
     AsmHelper.EmitAssembly(Action, std::move(OS));
 
   // Verify clang's TargetInfo DataLayout against the LLVM TargetMachine's
   // DataLayout.
   if (AsmHelper.TM) {