[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

clang/lib/CodeGen/CodeGenAction.cpp

if (!CI.getCodeGenOpts().ThinLTOIndexFile.empty()) {
     };
 
     auto CreateEmptyModule = [&]() -> std::unique_ptr<llvm::Module> {
       auto M = llvm::make_unique<llvm::Module>("empty", *VMContext);
       M->setTargetTriple(CI.getTargetOpts().Triple);
       return M;
     };
 
+    sys::fs::file_status Status;
+    if (auto E = sys::fs::status(CI.getCodeGenOpts().ThinLTOIndexFile, Status,
+                                 true)) {
+      return DiagErrors(make_error<StringError>(
+          "Error loading index file '" + CI.getCodeGenOpts().ThinLTOIndexFile +
+              "'",
+          E));
+    }
+    // Empty ThinLTOIndexFile signals that we don't need this module during
+    // linking. So we should not run ThinLTO backend even if it contains the
+    // ThinLTO module. Backend even may fail because of lack of necessary
+    // information which should be provided by ThinLTOIndex.
+    if (!Status.getSize())
+      return CreateEmptyModule();
+
     Expected<std::vector<BitcodeModule>> BMsOrErr = getBitcodeModuleList(MBRef);
     if (!BMsOrErr)
       return DiagErrors(BMsOrErr.takeError());
     BitcodeModule *Bm = FindThinLTOModule(*BMsOrErr);
     // We have nothing to do if the file contains no ThinLTO module. This is
     // possible if ThinLTO compilation was not able to split module. Content of
     // the file was already processed by indexing and will be passed to the
     // linker using merged object file.

clang/test/CodeGen/thinlto_backend.ll

 ; REQUIRES: x86-registered-target
 
 ; RUN: opt -module-summary -o %t1.o %s
 ; RUN: opt -module-summary -o %t2.o %S/Inputs/thinlto_backend.ll
 ; RUN: llvm-lto -thinlto -o %t %t1.o %t2.o
 
 ; Ensure clang -cc1 give expected error for incorrect input type
 ; RUN: not %clang_cc1 -O2 -o %t1.o -x c %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'
 
 ; Ensure we get expected error for missing index file
-; RUN: %clang -O2 -o %t4.o -x ir %t1.o -c -fthinlto-index=bad.thinlto.bc 2>&1 | FileCheck %s -check-prefix=CHECK-ERROR1
+; RUN: not %clang -O2 -o %t4.o -x ir %t1.o -c -fthinlto-index=bad.thinlto.bc 2>&1 | FileCheck %s -check-prefix=CHECK-ERROR1
 ; CHECK-ERROR1: Error loading index file 'bad.thinlto.bc'
 
 ; Ensure we ignore empty index file, and run non-ThinLTO compilation which
 ; would not import f2
 ; RUN: touch %t4.thinlto.bc
 ; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t4.o -x ir %t1.o -c -fthinlto-index=%t4.thinlto.bc
-; RUN: llvm-nm %t4.o | FileCheck --check-prefix=CHECK-OBJ-IGNORE-EMPTY %s
-; CHECK-OBJ-IGNORE-EMPTY: T f1
-; CHECK-OBJ-IGNORE-EMPTY: U f2
+; RUN: llvm-nm %t4.o | wc -l | FileCheck %s --check-prefix=CHECK-OBJ-EMPTY
 
 ; 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 | wc -l | FileCheck %s --check-prefix=CHECK-SIZE-EMPTY
 ; CHECK-SIZE-EMPTY: {{^0$}}