[ThinLTO/WPD] Remove dead code and improve test case

[evgeny777]

Summary

I was studying changes to BackendUtil introduced by D71913 and found out that

• Removing invocation of LTT pass in BackendUtil.cpp doesn't break any test case including thinlto-distributed-type-metadata.cpp
• When combined index doesn't have skipModuleByDistributedBackendflag we run normal thin LTO pipeline which removes type.test/assume sequences either in WPD or in LTT.
• When combined index does have skipModuleByDistributedBackend flag we end up with empty module without invoking backend.

So adding LTT pass in BackendUtil seems to be not needed.

[tejohnson]

Let me investigate and get back. I know I ran into issues when testing our apps with my changes that required this. But perhaps it is only the invocation at line 1180 - are you saying that one is needed but the others aren't?

[evgeny777]

I know I ran into issues when testing our apps with my changes that required this. But perhaps it is only the invocation at line 1180 - are you saying that one is needed but the others aren't?

I'm not sure it's needed as well. Most likely I simply missed it.
If I'm wrong about those invocations please submit/modify the test case.

[tejohnson]

In D74132#1861886, @evgeny777 wrote:

I know I ran into issues when testing our apps with my changes that required this. But perhaps it is only the invocation at line 1180 - are you saying that one is needed but the others aren't?

I'm not sure it's needed as well. Most likely I simply missed it.
If I'm wrong about those invocations please submit/modify the test case.

I had added some cases to clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp for these:

// Also check type test are lowered when the distributed ThinLTO backend clang
// invocation is passed an empty index file, in which case a non-ThinLTO
// compilation pipeline is invoked. If not lowered then LLVM CodeGen may assert.

If I remove the third LTT call in BackendUtil.cpp (corresponding to the new PM >O0), then it gets an assert:

Can't get register for value!
UNREACHABLE executed at /usr/local/google/home/tejohnson/llvm/llvm_m1/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1610!

I did also add invocations of the test there for the other paths (old PM, and newPM -O0), but apparently they don't hit the same assert. Even though I haven't been able to trigger the same failure in those cases, I think it makes sense to keep these paths consistent in terms of the code they will send through the LLVM pipeline.

Note that this case is different than what the skipModuleByDistributedBackend flag is indicating. That flag indicates that the linker decided not to include any symbols from the module. The case this is handling is when we have a module, but an empty index file (it was /dev/null). This is used in our distributed build in some cases where we want to compile the bitcode down to native code without applying any LTO optimizations (in certain cases for testing, or a few situations where ThinLTO isn't well supported). In that case there is no WPD/LTT as we do a non-ThinLTO pipeline.

[evgeny777]

Thanks!

clang/lib/CodeGen/BackendUtil.cpp

void EmitAssemblyHelper::CreatePasses(legacy::PassManager &MPM,
   // Figure out TargetLibraryInfo.  This needs to be added to MPM and FPM
   // manually (and not via PMBuilder), since some passes (eg. InstrProfiling)
   // are inserted before PMBuilder ones - they'd get the default-constructed
   // TLI with an unknown target otherwise.
   Triple TargetTriple(TheModule->getTargetTriple());
   std::unique_ptr<TargetLibraryInfoImpl> TLII(
       createTLII(TargetTriple, CodeGenOpts));
 
-  // If we reached here with a non-empty index file name, then the index file
-  // was empty and we are not performing ThinLTO backend compilation (used in
-  // testing in a distributed build environment). Drop any the type test
-  // assume sequences inserted for whole program vtables so that codegen doesn't
-  // complain.
-  if (!CodeGenOpts.ThinLTOIndexFile.empty())
-    MPM.add(createLowerTypeTestsPass(/*ExportSummary=*/nullptr,
-                                     /*ImportSummary=*/nullptr,
-                                     /*DropTypeTests=*/true));
-
   PassManagerBuilderWrapper PMBuilder(TargetTriple, CodeGenOpts, LangOpts);
 
   // At O0 and O1 we only run the always inliner which is more efficient. At
   // higher optimization levels we run the normal inliner.
   if (CodeGenOpts.OptimizationLevel <= 1) {
     bool InsertLifetimeIntrinsics = (CodeGenOpts.OptimizationLevel != 0 &&
                                      !CodeGenOpts.DisableLifetimeMarkers);
     PMBuilder.Inliner = createAlwaysInlinerLegacyPass(InsertLifetimeIntrinsics);
if (!CodeGenOpts.DisableLLVMPasses) {
     bool IsLTO = CodeGenOpts.PrepareForLTO;
 
     if (CodeGenOpts.OptimizationLevel == 0) {
       // If we reached here with a non-empty index file name, then the index
       // file was empty and we are not performing ThinLTO backend compilation
       // (used in testing in a distributed build environment). Drop any the type
       // test assume sequences inserted for whole program vtables so that
       // codegen doesn't complain.
-      if (!CodeGenOpts.ThinLTOIndexFile.empty())
-        MPM.addPass(LowerTypeTestsPass(/*ExportSummary=*/nullptr,
-                                       /*ImportSummary=*/nullptr,
-                                       /*DropTypeTests=*/true));
       if (Optional<GCOVOptions> Options = getGCOVOptions(CodeGenOpts))
         MPM.addPass(GCOVProfilerPass(*Options));
       if (Optional<InstrProfOptions> Options =
               getInstrProfOptions(CodeGenOpts, LangOpts))
         MPM.addPass(InstrProfiling(*Options, false));
 
       // Build a minimal pipeline based on the semantics required by Clang,
       // which is just that always inlining occurs. Further, disable generating

clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp

 // Test distributed ThinLTO backend handling of type tests
 
 // REQUIRES: x86-registered-target
 
 // Ensure that a distributed backend invocation of ThinLTO lowers the type test
 // as expected.
 // RUN: %clang_cc1 -flto=thin -flto-unit -triple x86_64-unknown-linux -fwhole-program-vtables -emit-llvm-bc -o %t.o %s
 // RUN: llvm-dis %t.o -o - | FileCheck --check-prefix=TT %s
 // RUN: llvm-lto -thinlto -o %t2 %t.o
 // RUN: %clang -target x86_64-unknown-linux -O2 -o %t3.o -x ir %t.o -c -fthinlto-index=%t2.thinlto.bc -save-temps=obj
 // RUN: llvm-dis %t.s.4.opt.bc -o - | FileCheck --check-prefix=OPT %s
 // llvm-nm %t3.o | FileCheck --check-prefix=NM %s
+// In case we skip module in a distributed backend we should end up with an empty module,
+// which doesn't have type.test/assume sequences.
+// RUN: llvm-lto -thinlto -skip-module-for-distributed-backend -o %t-skip %t.o
+// RUN: %clang -target x86_64-unknown-linux -O2 -o %t-empty.ll -x ir %t.o -S -emit-llvm -fthinlto-index=%t-skip.thinlto.bc
+// RUN: cat %t-empty.ll | FileCheck --check-prefix=EMPTY %s
 
 // The pre-link bitcode produced by clang should contain a type test assume
 // sequence.
 // TT: [[TTREG:%[0-9]+]] = call i1 @llvm.type.test({{.*}}, metadata !"_ZTS1A")
 // TT: void @llvm.assume(i1 [[TTREG]])
 
 // The ThinLTO backend optimized bitcode should not have any type test assume
 // sequences.
 // OPT-NOT: @llvm.type.test
 // OPT-NOT: call void @llvm.assume
 // We should have only one @llvm.assume call, the one that was expanded
 // from the builtin in the IR below, not the one fed by the type test.
 // OPT: %cmp = icmp ne %struct.A* %{{.*}}, null
 // OPT: void @llvm.assume(i1 %cmp)
 // Check after the builtin assume again that we don't have a type test assume
 // sequence.
 // OPT-NOT: @llvm.type.test
 // OPT-NOT: call void @llvm.assume
 
 // NM: T _Z2afP1A
 
+// EMPTY-NOT: @llvm.type.test
+// EMPTY-NOT: call void @llvm.assume
+
 // Also check type test are lowered when the distributed ThinLTO backend clang
 // invocation is passed an empty index file, in which case a non-ThinLTO
 // compilation pipeline is invoked. If not lowered then LLVM CodeGen may assert.
 // RUN: touch %t4.thinlto.bc
 // O2 old PM
 // RUN: %clang -target x86_64-unknown-linux -O2 -o %t4.o -x ir %t.o -c -fthinlto-index=%t4.thinlto.bc -save-temps=obj
 // RUN: llvm-dis %t.s.4.opt.bc -o - | FileCheck --check-prefix=OPT %s
 // llvm-nm %t4.o | FileCheck --check-prefix=NM %s

llvm/tools/llvm-lto/llvm-lto.cpp

 static cl::opt<bool> UseDiagnosticHandler(
     "use-diagnostic-handler", cl::init(false),
     cl::desc("Use a diagnostic handler to test the handler interface"));
 
 static cl::opt<bool>
     ThinLTO("thinlto", cl::init(false),
             cl::desc("Only write combined global index for ThinLTO backends"));
 
+static cl::opt<bool> SkipModuleForDistributedBackend(
+    "skip-module-for-distributed-backend", cl::init(false),
+    cl::desc("Request distributed backend to skip compilation of the module"));
+
 enum ThinLTOModes {
   THINLINK,
   THINDISTRIBUTE,
   THINEMITIMPORTS,
   THINPROMOTE,
   THINIMPORT,
   THININTERNALIZE,
   THINOPT,
static void createCombinedModuleSummaryIndex() {
   ModuleSummaryIndex CombinedIndex(/*HaveGVs=*/false);
   uint64_t NextModuleId = 0;
   for (auto &Filename : InputFilenames) {
     ExitOnError ExitOnErr("llvm-lto: error loading file '" + Filename + "': ");
     std::unique_ptr<MemoryBuffer> MB =
         ExitOnErr(errorOrToExpected(MemoryBuffer::getFileOrSTDIN(Filename)));
     ExitOnErr(readModuleSummaryIndex(*MB, CombinedIndex, NextModuleId++));
   }
+  if (SkipModuleForDistributedBackend)
+    CombinedIndex.setSkipModuleByDistributedBackend();
   std::error_code EC;
   assert(!OutputFilename.empty());
   raw_fd_ostream OS(OutputFilename + ".thinlto.bc", EC,
                     sys::fs::OpenFlags::OF_None);
   error(EC, "error opening the file '" + OutputFilename + ".thinlto.bc'");
   WriteIndexToFile(CombinedIndex, OS);
   OS.close();
 }