[ThinLTO] Remove dead and dropped symbol declarations when possible

[tejohnson]

Summary

Removing the dropped symbols will prevent indirect call promotion in the
ThinLTO Backend from adding a new reference to a symbol, which can
result in linker unsats. This can happen when we compile with a sample
profile collected from one binary by used for another, which may have
profiled targets that aren't used in the new binary.

Note that until dropDeadSymbols handles variables and aliases (in
progress), we may not be able to remove the declaration and can still
have an issue.

[grimar]

Looks good to me. Thanks !
(suggestion is below)

lib/LTO/LTOBackend.cpp

410

You can skip this check I think, removeDeadConstantUsers
would just do nothing in this case.

for (GlobalValue *GV : DeadGVs) {
  GV->removeDeadConstantUsers();

  // <comment>
  if (GV->use_empty())
    GV->eraseFromParent();
}

[tejohnson]

Update to reflect changes to drop dead variables and aliases

[tejohnson]

Address comments

lib/LTO/LTOBackend.cpp

splitCodeGen(C, TM.get(), AddStream, ParallelCodeGenParallelismLevel,
                  std::move(Mod));
   }
   finalizeOptimizationRemarks(std::move(DiagnosticOutputFile));
   return Error::success();
 }
 
 static void dropDeadSymbols(Module &Mod, const GVSummaryMapTy &DefinedGlobals,
                             const ModuleSummaryIndex &Index) {
-  std::vector<GlobalValue *> ReplacedGlobals;
+  std::vector<GlobalValue*> DeadGVs;
   for (auto &GV : Mod.global_values())
     if (GlobalValueSummary *GVS = DefinedGlobals.lookup(GV.getGUID()))
-      if (!Index.isGlobalValueLive(GVS) && !convertToDeclaration(GV))
-        ReplacedGlobals.push_back(&GV);
+      if (!Index.isGlobalValueLive(GVS)) {
+        DeadGVs.push_back(&GV);
+        convertToDeclaration(GV);
+      }
 
-  for (GlobalValue *GV : ReplacedGlobals)
+  // Now that all dead bodies have been dropped, delete the actual objects
+  // themselves when possible.
+  for (GlobalValue *GV : DeadGVs) {
+    GV->removeDeadConstantUsers();
+    // Might reference something defined in native object (i.e. dropped a
+    // non-prevailing IR def, but we need to keep the declaration).
+    if (GV->use_empty())
       GV->eraseFromParent();
   }
+}
 
 Error lto::thinBackend(Config &Conf, unsigned Task, AddStreamFn AddStream,
                        Module &Mod, const ModuleSummaryIndex &CombinedIndex,
                        const FunctionImporter::ImportMapTy &ImportList,
                        const GVSummaryMapTy &DefinedGlobals,
                        MapVector<StringRef, BitcodeModule> &ModuleMap) {
   Expected<const Target *> TOrErr = initAndLookupTarget(Conf, Mod);
   if (!TOrErr)

test/ThinLTO/X86/deadstrip.ll

 ; CHECK: define internal void @bar_internal()
 ; CHECK: define internal void @dead_func() {
 ; CHECK-NOT: available_externally {{.*}} @baz()
 ; LTO2-NOT: available_externally {{.*}} @baz()
 ; LTO2: @llvm.global_ctors =
 ; LTO2: define internal void @_GLOBAL__I_a()
 ; LTO2: define internal void @bar() {
 ; LTO2: define internal void @bar_internal()
-; LTO2: declare dso_local void @dead_func()
+; LTO2-NOT: @dead_func()
 ; LTO2-NOT: available_externally {{.*}} @baz()
 
 ; Make sure we didn't internalize @boo, which is reachable via
 ; llvm.global_ctors
 ; CHECK2: define void @boo()
 ; LTO2-CHECK2: define dso_local void @boo()
 ; We should have eventually removed @baz since it was internalized and unused
 ; CHECK2-NM-NOT: _baz

test/Transforms/PGOProfile/thinlto_samplepgo_icp_droppeddead.ll

+; REQUIRES: x86-registered-target
+
+; RUN: opt -module-summary %s -o %t.bc
+
+; Tests that with dead stripping in the thin link enabled (default), we do not
+; promote to target of the dropped dead symbol _ZL3foov. This can happen with a
+; sample profile collected for one binary used to  optimize for another binary.
+; RUN: llvm-lto2 run -save-temps -o %t2 %t.bc -r %t.bc,fptr,plx \
+; RUN:    -r %t.bc,main,plx -r %t.bc,_ZL3foov,l
+; RUN: llvm-dis < %t2.1.4.opt.bc | FileCheck %s --check-prefix=OPT
+; RUN: llvm-lto2 run -save-temps -o %t2 %t.bc -r %t.bc,fptr,plx \
+; RUN:    -r %t.bc,main,plx -r %t.bc,_ZL3foov,l -compute-dead=false
+; RUN: llvm-dis < %t2.1.4.opt.bc | FileCheck %s --check-prefix=OPT-NODEAD
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@fptr = local_unnamed_addr global void ()* null, align 8
+
+define void @_ZL3foov() #1 {
+entry:
+  ret void
+}
+
+define i32 @main() local_unnamed_addr #0 !prof !34 {
+entry:
+  %0 = load void ()*, void ()** @fptr, align 8
+; OPT-NOT: label %if.false.orig_indirect
+; OPT-NODEAD: br i1 %{{[0-9]+}}, label %if.end.icp, label %if.false.orig_indirect
+  tail call void %0(), !prof !40
+  ret i32 0
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3,!4}
+!llvm.ident = !{!31}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 5.0.0 (trunk 297016)", isOptimized: true, runtimeVersion: 0, emissionKind: NoDebug, enums: !2)
+!1 = !DIFile(filename: "main.cc", directory: ".")
+!2 = !{}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"ProfileSummary", !5}
+!5 = !{!6, !7, !8, !9, !10, !11, !12, !13}
+!6 = !{!"ProfileFormat", !"SampleProfile"}
+!7 = !{!"TotalCount", i64 3003}
+!8 = !{!"MaxCount", i64 3000}
+!9 = !{!"MaxInternalCount", i64 0}
+!10 = !{!"MaxFunctionCount", i64 0}
+!11 = !{!"NumCounts", i64 3}
+!12 = !{!"NumFunctions", i64 1}
+!13 = !{!"DetailedSummary", !14}
+!14 = !{!15, !16, !17, !18, !19, !20, !20, !21, !21, !22, !23, !24, !25, !26, !27, !28, !29, !30}
+!15 = !{i32 10000, i64 3000, i32 1}
+!16 = !{i32 100000, i64 3000, i32 1}
+!17 = !{i32 200000, i64 3000, i32 1}
+!18 = !{i32 300000, i64 3000, i32 1}
+!19 = !{i32 400000, i64 3000, i32 1}
+!20 = !{i32 500000, i64 3000, i32 1}
+!21 = !{i32 600000, i64 3000, i32 1}
+!22 = !{i32 700000, i64 3000, i32 1}
+!23 = !{i32 800000, i64 3000, i32 1}
+!24 = !{i32 900000, i64 3000, i32 1}
+!25 = !{i32 950000, i64 3000, i32 1}
+!26 = !{i32 990000, i64 3000, i32 1}
+!27 = !{i32 999000, i64 3000, i32 1}
+!28 = !{i32 999900, i64 2, i32 2}
+!29 = !{i32 999990, i64 2, i32 2}
+!30 = !{i32 999999, i64 2, i32 2}
+!31 = !{!"clang version 5.0.0 (trunk 297016)"}
+!34 = !{!"function_entry_count", i64 1}
+!40 = !{!"VP", i32 0, i64 3000, i64 -8789629626369651636, i64 3000}

test/tools/gold/X86/global_with_section.ll

 ; We should not internalize @var_with_section due to section
 ; CHECK-DAG: @var_with_section = dso_local global i32 0, section "some_section"
 @var_with_section = global i32 0, section "some_section"
 
 ; Confirm via a variable with a non-C identifier section that we are getting
 ; the expected internalization.
 ; CHECK-REGULARLTO-DAG: @var_with_nonC_section = internal global i32 0, section ".nonCsection"
 ; Check we dropped definition of dead variable.
-; CHECK-THINLTO-DAG: @var_with_nonC_section = external dso_local global i32, section ".nonCsection"
+; CHECK-THINLTO-NOT: @var_with_nonC_section
 @var_with_nonC_section = global i32 0, section ".nonCsection"
 
 ; We should not internalize @deadfunc_with_section due to section
 ; CHECK-DAG: define dso_local void @deadfunc_with_section() section "some_other_section"
 define void @deadfunc_with_section() section "some_other_section" {
   call void @deadfunc2_called_from_section()
   ret void
 }
 
 ; Confirm via a function with a non-C identifier section that we are getting
 ; the expected internalization.
 ; CHECK2-REGULARLTO-DAG: define internal void @deadfunc_with_nonC_section() section ".nonCsection"
 ; Check dead function converted to declaration.
-; CHECK-THINLTO-DAG: declare dso_local void @deadfunc_with_nonC_section() section ".nonCsection"
+; CHECK-THINLTO-NOT: @deadfunc_with_nonC_section()
 define void @deadfunc_with_nonC_section() section ".nonCsection" {
   call void @deadfunc2_called_from_nonC_section()
   ret void
 }
 
 ; In RegularLTO mode, where we have combined all the IR,
 ; @deadfunc2_called_from_section can be internalized.
 ; CHECK2-REGULARLTO: define internal void @deadfunc2_called_from_section
 ; In ThinLTO mode, we can't internalize it as it needs to be preserved
 ; (due to the access from @deadfunc_with_section which must be preserved), and
 ; can't be internalized since the reference is from a different module.
 ; CHECK2-THINLTO: define dso_local void @deadfunc2_called_from_section
 declare void @deadfunc2_called_from_section()
 
 ; Confirm when called from a function with a non-C identifier section that we
 ; are getting the expected internalization.
 ; CHECK2-REGULARLTO: define internal void @deadfunc2_called_from_nonC_section
 ; Check dead function converted to declaration.
-; CHECK2-THINLTO: declare dso_local void @deadfunc2_called_from_nonC_section
+; CHECK2-THINLTO-NOT: @deadfunc2_called_from_nonC_section
 declare void @deadfunc2_called_from_nonC_section()