[ThinLTO] Convert dead alias to declarations

[tejohnson]

Summary

This complements the fixes in r323633 and r324075 which drop the
definitions of dead functions and variables, respectively.

Fixes PR36208.

[grimar]

lib/LTO/LTOBackend.cpp

414

With the new change convertToDeclaration becomes a dangerous function to use in loops.

Currently there is one more place where code iterates over aliases in a "old" way and may also fail I think:
https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/FunctionImport.cpp#L670
https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/FunctionImport.cpp#L689

What about if we stop calling eraseFromParent from convertToDeclatation
and do something explicit like:

static void dropDeadSymbols(Module &Mod, const GVSummaryMapTy &DefinedGlobals,
                            const ModuleSummaryIndex &Index) {
  std::vector<GlobalValue *> ReplacedGlobals;
  for (auto &GV : Mod.global_values())
    if (GlobalValueSummary *GVS = DefinedGlobals.lookup(GV.getGUID()))
      if (!Index.isGlobalValueLive(GVS))
        if (!convertToDeclaration(GV))
          ReplacedGlobals.push_back(&GV);

  for (GlobalValue *GV : ReplacedGlobals)
    GV->eraseFromParent();
}

So convertToDeclaration would return true if value was converted to declaration and
false if it was replaced.

Though we can refactor this place separatelly in a followups, probably with use of
some better approach, but at least caller from FunctionImport.cpp should also be changed too then.

lib/Transforms/IPO/FunctionImport.cpp

634

Looks else part is not covered by any tests atm.
(I replaced it with else assert(false) and check-all showed no failtures).

Suggest to add variable with alias to your testcase
and check them are converted to declarations, like:

; RUN:   -r %t1.bc,var,x -r %t1.bc,varAlias,x \
; RUN:   -o %t2.o -save-temps
...
; DROP-DAG: @var = external global i32
; DROP-DAG: @varAlias = external global i32
...
@var = global i32 99
@varAlias = alias i32, i32* @var

[tejohnson]

lib/LTO/LTOBackend.cpp

414

I changed as suggested as that is less problematic for other callsites.
Note that there won't currently be a problem at the other callsite since it doesn't call convertToDeclaration with an alias (otherwise it previously would have hit the lllvm_unreachable). I added an assert there that convertToDeclaration returns true, and a fixme that it should be changed once thinLTOResolveWeakForLinkerGUID is changed to handle aliases (which it can now be with this change in convertToDeclaration).

[tejohnson]

Address comments

[grimar]

LGTM, thanks !
(minor suggestions below)

lib/LTO/LTOBackend.cpp

410

Please format (it should be 2 lines after clang-formatting I think).

414

Note that there won't currently be a problem at the other callsite since it doesn't call convertToDeclaration with an alias > (otherwise it previously would have hit the llvm_unreachable).

Probably yes, though it is also seems can be possible that we do not have such test that would go that path to hit unreachable at this moment.

lib/Transforms/IPO/FunctionImport.cpp

684

I suggest to use llvm_unreachable as we explicitly do not support/implement this path
while assert will be just eliminated in release builds.

if (!convertToDeclaration(GV))
  llvm_unreachable("Expected GV to be converted");

[tejohnson]

lib/LTO/LTOBackend.cpp

410

clang-format did this, but it turns out it had the wrong style set as my default. Fixed.

[tejohnson]

Address comments

include/llvm/Transforms/IPO/FunctionImport.h

 /// \p GUIDPreservedSymbols. Non-prevailing symbols are symbols without a
 /// prevailing copy anywhere in IR and are normally dead, \p isPrevailing
 /// predicate returns status of symbol.
 void computeDeadSymbols(
     ModuleSummaryIndex &Index,
     const DenseSet<GlobalValue::GUID> &GUIDPreservedSymbols,
     function_ref<PrevailingType(GlobalValue::GUID)> isPrevailing);
 
-/// Converts value \p GV to declaration.
-void convertToDeclaration(GlobalValue &GV);
+/// Converts value \p GV to declaration, or replaces with a declaration if
+/// it is an alias. Returns true if converted, false if replaced.
+bool convertToDeclaration(GlobalValue &GV);
 
 /// Compute the set of summaries needed for a ThinLTO backend compilation of
 /// \p ModulePath.
 //
 /// This includes summaries from that module (in case any global summary based
 /// optimizations were recorded) and from any definitions in other modules that
 /// should be imported.
 //

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) {
-  auto MaybeDrop = [&](GlobalValue &GV) {
+  std::vector<GlobalValue *> ReplacedGlobals;
+  for (auto &GV : Mod.global_values())
     if (GlobalValueSummary *GVS = DefinedGlobals.lookup(GV.getGUID()))
-      if (!Index.isGlobalValueLive(GVS))
-        convertToDeclaration(GV);
-  };
+      if (!Index.isGlobalValueLive(GVS) && !convertToDeclaration(GV))
+        ReplacedGlobals.push_back(&GV);
 
-  // Process functions and global now.
-  // FIXME: add support for aliases (needs support in convertToDeclaration).
-  for (auto &GV : Mod)
-    MaybeDrop(GV);
-  for (auto &GV : Mod.globals())
-    MaybeDrop(GV);
+  for (GlobalValue *GV : ReplacedGlobals) 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)
     return TOrErr.takeError();
 
   std::unique_ptr<TargetMachine> TM = createTargetMachine(Conf, *TOrErr, Mod);

lib/Transforms/IPO/FunctionImport.cpp

llvm::EmitImportsFiles(StringRef ModulePath, StringRef OutputFilename,
   raw_fd_ostream ImportsOS(OutputFilename, EC, sys::fs::OpenFlags::F_None);
   if (EC)
     return EC;
   for (auto &ILI : ModuleImports)
     ImportsOS << ILI.first() << "\n";
   return std::error_code();
 }
 
-void llvm::convertToDeclaration(GlobalValue &GV) {
+bool llvm::convertToDeclaration(GlobalValue &GV) {
   DEBUG(dbgs() << "Converting to a declaration: `" << GV.getName() << "\n");
   if (Function *F = dyn_cast<Function>(&GV)) {
     F->deleteBody();
     F->clearMetadata();
     F->setComdat(nullptr);
   } else if (GlobalVariable *V = dyn_cast<GlobalVariable>(&GV)) {
     V->setInitializer(nullptr);
     V->setLinkage(GlobalValue::ExternalLinkage);
     V->clearMetadata();
     V->setComdat(nullptr);
-  } else
-    // For now we don't resolve or drop aliases. Once we do we'll
-    // need to add support here for creating either a function or
-    // variable declaration, and return the new GlobalValue* for
-    // the caller to use.
-    // Support of dropping aliases is required for correct dead code
-    // elimination performed in thin LTO backends (see 'dropDeadSymbols').
-    llvm_unreachable("Expected function or variable");
+  } else {
+    GlobalValue *NewGV;
+    if (GV.getValueType()->isFunctionTy())
+      NewGV =
+          Function::Create(cast<FunctionType>(GV.getValueType()),
+                           GlobalValue::ExternalLinkage, "", GV.getParent());
+    else
+      NewGV =
+          new GlobalVariable(*GV.getParent(), GV.getValueType(),
+                             /*isConstant*/ false, GlobalValue::ExternalLinkage,
+                             /*init*/ nullptr, "",
+                             /*insertbefore*/ nullptr, GV.getThreadLocalMode(),
+                             GV.getType()->getAddressSpace());
+    NewGV->takeName(&GV);
+    GV.replaceAllUsesWith(NewGV);
+    return false;
+  }
+  return true;
 }
 
 /// Fixup WeakForLinker linkages in \p TheModule based on summary analysis.
 void llvm::thinLTOResolveWeakForLinkerModule(
     Module &TheModule, const GVSummaryMapTy &DefinedGlobals) {
   auto updateLinkage = [&](GlobalValue &GV) {
     // See if the global summary analysis computed a new resolved linkage.
     const auto &GS = DefinedGlobals.find(GV.getGUID());
auto updateLinkage = [&](GlobalValue &GV) {
     if (!GlobalValue::isWeakForLinker(GV.getLinkage()))
       return;
     // Check for a non-prevailing def that has interposable linkage
     // (e.g. non-odr weak or linkonce). In that case we can't simply
     // convert to available_externally, since it would lose the
     // interposable property and possibly get inlined. Simply drop
     // the definition in that case.
     if (GlobalValue::isAvailableExternallyLinkage(NewLinkage) &&
-        GlobalValue::isInterposableLinkage(GV.getLinkage()))
-      convertToDeclaration(GV);
-    else {
+        GlobalValue::isInterposableLinkage(GV.getLinkage())) {
+      auto Converted = convertToDeclaration(GV);
+      // FIXME: Change this to collect replaced GVs and later erase
+      // them from the parent module once thinLTOResolveWeakForLinkerGUID is
+      // changed to enable this for aliases.
+      assert(Converted && "Expected GV to be converted");
+    } else {
       DEBUG(dbgs() << "ODR fixing up linkage for `" << GV.getName() << "` from "
                    << GV.getLinkage() << " to " << NewLinkage << "\n");
       GV.setLinkage(NewLinkage);
     }
     // Remove declarations from comdats, including available_externally
     // as this is a declaration for the linker, and will be dropped eventually.
     // It is illegal for comdats to contain declarations.
     auto *GO = dyn_cast_or_null<GlobalObject>(&GV);

test/LTO/Resolution/X86/not-prevailing-alias.ll

+; Test to ensure that dead alias are dropped by converting to a declaration
+; RUN: opt -module-summary %s -o %t1.bc
+; RUN: llvm-lto2 run %t1.bc -r %t1.bc,barAlias,x \
+; RUN:   -r %t1.bc,bar,x -r %t1.bc,zed,px \
+; RUN:   -r %t1.bc,var,x -r %t1.bc,varAlias,x \
+; RUN:   -o %t2.o -save-temps
+
+; Check that bar and barAlias were dropped to declarations
+; RUN: llvm-dis %t2.o.1.1.promote.bc -o - | FileCheck %s --check-prefix=DROP
+; DROP-DAG: declare void @bar()
+; DROP-DAG: declare void @barAlias()
+; DROP-DAG: @var = external global i32
+; DROP-DAG: @varAlias = external global i32
+
+; Check that 'barAlias' and 'varAlias' were not inlined.
+; RUN: llvm-objdump -d %t2.o.1 | FileCheck %s
+; CHECK:      zed:
+; CHECK-NEXT:  {{.*}}  pushq
+; CHECK-NEXT:  {{.*}}  callq   0
+; CHECK-NEXT:   movq  (%rip), %rax
+
+; Check that 'barAlias' and 'varAlias' produced as undefined.
+; RUN: llvm-readelf --symbols %t2.o.1 | FileCheck %s --check-prefix=SYMBOLS
+; SYMBOLS: NOTYPE  GLOBAL DEFAULT  UND barAlias
+; SYMBOLS: NOTYPE  GLOBAL DEFAULT  UND varAlias
+; SYMBOLS: FUNC    GLOBAL DEFAULT    2 zed
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@barAlias = alias void(), void()* @bar
+define void @bar() {
+  ret void
+}
+
+@var = global i32 99
+@varAlias = alias i32, i32* @var
+
+define i32 @zed() {
+  call void @barAlias()
+  %1 = load i32, i32* @varAlias, align 4
+  ret i32 %1
+}