[ThinLTO] Allow indexing to request backend to ignore the module

[vitalybuka]

Summary

Gold plugin does not add pass to ThinLTO modules without useful symbols.
In this case ThinLTO can't create corresponding index file and some features, like CFI,
cannot be processes by backed correctly without index.
Given that we don't need the backed output we can request it to avoid
processing the module. This is implemented by this patch using new "SkipModule"
flag.

[tejohnson]

Empty ThinLTOIndexFile signals that we don't need this module during
linking.

Not the only case actually. We now also pass an empty index file when we want to compile the bitcode down to object without applying any LTO optimization (there are a few cases where we decide we want to turn off LTO optimizations for some links), and this is currently relying on being able to pass /dev/null for the index file that would be broken by this change.

So we should not run ThinLTO backend even if it contains the
ThinLTO module. Backend may fail because of lack of necessary
information which should be provided by ThinLTOIndex.

This shouldn't happen - are you seeing cases where we fail? After loadModule() is called, EmitBackendOutput() is called which passes /*IgnoreEmptyThinLTOIndexFile*/true to getModuleSummaryIndexForFile, which would cause it to return nullptr if the index file is empty. Back in EmitBackendOutput(), if the combined index is null we will skip ThinLTO compilation and fall back to normal compilation.

[vitalybuka]

In D42995#1000025, @tejohnson wrote:

Empty ThinLTOIndexFile signals that we don't need this module during
linking.

Not the only case actually. We now also pass an empty index file when we want to compile the bitcode down to object without applying any LTO optimization (there are a few cases where we decide we want to turn off LTO optimizations for some links), and this is currently relying on being able to pass /dev/null for the index file that would be broken by this change.

I'd expect this should be done by indexing and content is already in the merged object file.
Not sure how to reproduce this. I've build some large targets and I never seen this.

So we should not run ThinLTO backend even if it contains the
ThinLTO module. Backend may fail because of lack of necessary
information which should be provided by ThinLTOIndex.

This shouldn't happen - are you seeing cases where we fail? After loadModule() is called, EmitBackendOutput() is called which passes /*IgnoreEmptyThinLTOIndexFile*/true to getModuleSummaryIndexForFile, which would cause it to return nullptr if the index file is empty. Back in EmitBackendOutput(), if the combined index is null we will skip ThinLTO compilation and fall back to normal compilation.

I don't see for regular compilation, but I see for for CFI. Backend will not be able to process llvm.type.test without TypeIdMap from index and it will crash in "Instruction Select"

[vitalybuka]

In D42995#1000155, @vitalybuka wrote:

In D42995#1000025, @tejohnson wrote:

Empty ThinLTOIndexFile signals that we don't need this module during
linking.

Not the only case actually. We now also pass an empty index file when we want to compile the bitcode down to object without applying any LTO optimization (there are a few cases where we decide we want to turn off LTO optimizations for some links), and this is currently relying on being able to pass /dev/null for the index file that would be broken by this change.

I'd expect this should be done by indexing and content is already in the merged object file.
Not sure how to reproduce this. I've build some large targets and I never seen this.

At least with gold I don't see how this possible. I see that thinlto.bc can be empty only
if getSymbolsAndView returns nullptr or if LTOInfo for input object was false.
Former means that we don't need this object and so I created this patch.
For latter we already going to do the same anyway: D42680

[tejohnson]

In D42995#1000155, @vitalybuka wrote:

In D42995#1000025, @tejohnson wrote:

Empty ThinLTOIndexFile signals that we don't need this module during
linking.

Not the only case actually. We now also pass an empty index file when we want to compile the bitcode down to object without applying any LTO optimization (there are a few cases where we decide we want to turn off LTO optimizations for some links), and this is currently relying on being able to pass /dev/null for the index file that would be broken by this change.

I'd expect this should be done by indexing and content is already in the merged object file.
Not sure how to reproduce this. I've build some large targets and I never seen this.

This is done in bazel, not in gold. See below.

So we should not run ThinLTO backend even if it contains the
ThinLTO module. Backend may fail because of lack of necessary
information which should be provided by ThinLTOIndex.

This shouldn't happen - are you seeing cases where we fail? After loadModule() is called, EmitBackendOutput() is called which passes /*IgnoreEmptyThinLTOIndexFile*/true to getModuleSummaryIndexForFile, which would cause it to return nullptr if the index file is empty. Back in EmitBackendOutput(), if the combined index is null we will skip ThinLTO compilation and fall back to normal compilation.

I don't see for regular compilation, but I see for for CFI. Backend will not be able to process llvm.type.test without TypeIdMap from index and it will crash in "Instruction Select"

@pcc and I already discussed this a little bit in the context of bazel. I will point you to that discussion separately.

[vitalybuka]

[vitalybuka]

Don't use empty index file

[tejohnson]

clang/lib/CodeGen/BackendUtil.cpp

1176 ↗

(On Diff #134385)

s/generated/generate an/

clang/test/CodeGen/thinlto-distributed-backend-skip.ll

3 ↗

(On Diff #134385)

s/backed/backend/

llvm/include/llvm/IR/ModuleSummaryIndex.h

685 ↗

(On Diff #134385)

Need to document clearly that this only applies to the distributed index case, and what it means to "skip" the module (i.e. skip the backend compilation and generate an empty module instead).

Maybe rename to something like SkipModuleDistributedBackend or something else very explicit to avoid confusion?

llvm/tools/gold/gold-plugin.cpp

819 ↗

(On Diff #134385)

Rename Skip to SkipModule to match the above comment documenting parameter

890 ↗

(On Diff #134385)

Think this should be "else if (options::thinlto_index_only) {"
i.e. we don't need/want to write these when we're in single process mode.

892 ↗

(On Diff #134385)

Document constant parameters here and in other places. e.g. "/* SkipModule= */ true"

[vitalybuka]

addressed comments

[tejohnson]

LGTM

lib/CodeGen/BackendUtil.cpp

 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) {
+  std::unique_ptr<llvm::Module> EmptyModule;
   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,
                                            /*IgnoreEmptyThinLTOIndexFile*/true);
     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) {
+    if (CombinedIndex) {
+      if (!CombinedIndex->skipModuleByDistributedBackend()) {
         runThinLTOBackend(CombinedIndex.get(), M, HeaderOpts, CGOpts, TOpts,
-                        LOpts, std::move(OS), CGOpts.SampleProfileFile, Action);
+                          LOpts, std::move(OS), CGOpts.SampleProfileFile,
+                          Action);
         return;
       }
+      // Distributed indexing detected that nothing from the module is needed
+      // for the final linking. So we can skip the compilation. We sill need to
+      // output an empty object file to make sure that a linker does not fail
+      // trying to read it. Also for some features, like CFI, we must skip
+      // the compilation as CombinedIndex does not contain all required
+      // information.
+      EmptyModule = llvm::make_unique<llvm::Module>("empty", M->getContext());
+      EmptyModule->setTargetTriple(M->getTargetTriple());
+      M = EmptyModule.get();
+    }
   }
 
   EmitAssemblyHelper AsmHelper(Diags, HeaderOpts, CGOpts, TOpts, LOpts, M);
 
   if (CGOpts.ExperimentalNewPassManager)
     AsmHelper.EmitAssemblyWithNewPassManager(Action, std::move(OS));
   else
     AsmHelper.EmitAssembly(Action, std::move(OS));

test/CodeGen/thinlto-distributed-backend-skip.ll

+; REQUIRES: x86-registered-target
+
+; Check that ThinLTO backend respects "SkipModuleByDistributedBackend"
+; flag which can be set by indexing.
+
+; RUN: opt -thinlto-bc -o %t.o %s
+
+; RUN: %clang_cc1 -triple x86_64-grtev4-linux-gnu \
+; RUN:   -fthinlto-index=%S/Inputs/thinlto-distributed-backend-skip.bc \
+; RUN:   -emit-llvm -o - -x ir %t.o | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-grtev4-linux-gnu"
+
+; CHECK: "empty"
+; CHECK: target triple =
+; CHECK-NOT: @main
+define i32 @main() {
+entry:
+  ret i32 0
+}