[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–414

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

685

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–414

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

[tejohnson]

Address comments

lib/LTO/LTOBackend.cpp

 static void dropDeadSymbols(Module &Mod, const GVSummaryMapTy &DefinedGlobals,
                             const ModuleSummaryIndex &Index) {
   auto MaybeDrop = [&](GlobalValue &GV) {
     if (GlobalValueSummary *GVS = DefinedGlobals.lookup(GV.getGUID()))
       if (!Index.isGlobalValueLive(GVS))
         convertToDeclaration(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 (Module::global_value_iterator I = Mod.global_value_begin(),
+                                     E = Mod.global_value_end();
+       I != E;)
+    // Increment iterator here since MaybeDrop can delete aliases.
+    MaybeDrop(*I++);
 }
 
 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);

lib/Transforms/IPO/FunctionImport.cpp

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);
+    GV.eraseFromParent();
+  }
 }
 
 /// 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::isAvailableExternallyLinkage(NewLinkage) &&
         GlobalValue::isInterposableLinkage(GV.getLinkage()))
       convertToDeclaration(GV);
     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);
     if (GO && GO->isDeclarationForLinker() && GO->hasComdat())
       GO->setComdat(nullptr);
   };
 
   // Process functions and global now

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:   -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()
+
+; Check that 'barAlias' was not inlined.
+; RUN: llvm-objdump -d %t2.o.1 | FileCheck %s
+; CHECK:      zed:
+; CHECK-NEXT:  {{.*}}  pushq
+; CHECK-NEXT:  {{.*}}  callq   0
+
+; Check that 'bar' produced as undefined.
+; RUN: llvm-readelf --symbols %t2.o.1 | FileCheck %s --check-prefix=SYMBOLS
+; SYMBOLS: NOTYPE  GLOBAL DEFAULT  UND barAlias
+; 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
+}
+
+define i32 @zed() {
+  call void @barAlias()
+  ret i32 1
+}