Internalize: internalize comdat members as a group, and drop comdat o…

…n such members.

Internalizing an individual comdat group member without also internalizing
the other members of the comdat can break comdat semantics. For example,
if a module contains a reference to an internalized comdat member, and the
linker chooses a comdat group from a different object file, this will break
the reference to the internalized member.

This change causes the internalizer to only internalize comdat members if all
other members of the comdat are not externally visible. Once a comdat group
has been fully internalized, there is no need to apply comdat rules to its
members; later optimization passes (e.g. globaldce) can legally drop individual
members of the comdat. So we drop the comdat attribute from all comdat members.

Differential Revision: http://reviews.llvm.org/D10679

llvm-svn: 242423

llvm/lib/Transforms/IPO/Internalize.cpp

@@ -60,6 +60,10 @@ namespace {
     explicit InternalizePass();
     explicit InternalizePass(ArrayRef<const char *> ExportList);
     void LoadFile(const char *Filename);
+    bool maybeInternalize(GlobalValue &GV,
+                          const std::set<const Comdat *> &ExternalComdats);
+    void checkComdatVisibility(GlobalValue &GV,
+                               std::set<const Comdat *> &ExternalComdats);
     bool runOnModule(Module &M) override;
 
     void getAnalysisUsage(AnalysisUsage &AU) const override {
@@ -105,40 +109,85 @@ void InternalizePass::LoadFile(const char *Filename) {
   }
 }
 
-static bool shouldInternalize(const GlobalValue &GV,
-                              const std::set<std::string> &ExternalNames) {
+static bool isExternallyVisible(const GlobalValue &GV,
+                                const std::set<std::string> &ExternalNames) {
   // Function must be defined here
   if (GV.isDeclaration())
-    return false;
+    return true;
 
   // Available externally is really just a "declaration with a body".
   if (GV.hasAvailableExternallyLinkage())
-    return false;
+    return true;
 
   // Assume that dllexported symbols are referenced elsewhere
   if (GV.hasDLLExportStorageClass())
-    return false;
-
-  // Already has internal linkage
-  if (GV.hasLocalLinkage())
-    return false;
+    return true;
 
   // Marked to keep external?
-  if (ExternalNames.count(GV.getName()))
-    return false;
+  if (!GV.hasLocalLinkage() && ExternalNames.count(GV.getName()))
+    return true;
+
+  return false;
+}
 
+// Internalize GV if it is possible to do so, i.e. it is not externally visible
+// and is not a member of an externally visible comdat.
+bool InternalizePass::maybeInternalize(
+    GlobalValue &GV, const std::set<const Comdat *> &ExternalComdats) {
+  if (Comdat *C = GV.getComdat()) {
+    if (ExternalComdats.count(C))
+      return false;
+
+    // If a comdat is not externally visible we can drop it.
+    if (auto GO = dyn_cast<GlobalObject>(&GV))
+      GO->setComdat(nullptr);
+
+    if (GV.hasLocalLinkage())
+      return false;
+  } else {
+    if (GV.hasLocalLinkage())
+      return false;
+
+    if (isExternallyVisible(GV, ExternalNames))
+      return false;
+  }
+
+  GV.setVisibility(GlobalValue::DefaultVisibility);
+  GV.setLinkage(GlobalValue::InternalLinkage);
   return true;
 }
 
+// If GV is part of a comdat and is externally visible, keep track of its
+// comdat so that we don't internalize any of its members.
+void InternalizePass::checkComdatVisibility(
+    GlobalValue &GV, std::set<const Comdat *> &ExternalComdats) {
+  Comdat *C = GV.getComdat();
+  if (!C)
+    return;
+
+  if (isExternallyVisible(GV, ExternalNames))
+    ExternalComdats.insert(C);
+}
+
 bool InternalizePass::runOnModule(Module &M) {
   CallGraphWrapperPass *CGPass = getAnalysisIfAvailable<CallGraphWrapperPass>();
   CallGraph *CG = CGPass ? &CGPass->getCallGraph() : nullptr;
   CallGraphNode *ExternalNode = CG ? CG->getExternalCallingNode() : nullptr;
-  bool Changed = false;
 
   SmallPtrSet<GlobalValue *, 8> Used;
   collectUsedGlobalVariables(M, Used, false);
 
+  // Collect comdat visiblity information for the module.
+  std::set<const Comdat *> ExternalComdats;
+  if (!M.getComdatSymbolTable().empty()) {
+    for (Function &F : M)
+      checkComdatVisibility(F, ExternalComdats);
+    for (GlobalVariable &GV : M.globals())
+      checkComdatVisibility(GV, ExternalComdats);
+    for (GlobalAlias &GA : M.aliases())
+      checkComdatVisibility(GA, ExternalComdats);
+  }
+
   // We must assume that globals in llvm.used have a reference that not even
   // the linker can see, so we don't internalize them.
   // For llvm.compiler.used the situation is a bit fuzzy. The assembler and
@@ -154,17 +203,13 @@ bool InternalizePass::runOnModule(Module &M) {
 
   // Mark all functions not in the api as internal.
   for (Module::iterator I = M.begin(), E = M.end(); I != E; ++I) {
-    if (!shouldInternalize(*I, ExternalNames))
+    if (!maybeInternalize(*I, ExternalComdats))
       continue;
 
-    I->setVisibility(GlobalValue::DefaultVisibility);
-    I->setLinkage(GlobalValue::InternalLinkage);
-
     if (ExternalNode)
       // Remove a callgraph edge from the external node to this function.
       ExternalNode->removeOneAbstractEdgeTo((*CG)[I]);
 
-    Changed = true;
     ++NumFunctions;
     DEBUG(dbgs() << "Internalizing func " << I->getName() << "\n");
   }
@@ -191,30 +236,30 @@ bool InternalizePass::runOnModule(Module &M) {
   // internal as well.
   for (Module::global_iterator I = M.global_begin(), E = M.global_end();
        I != E; ++I) {
-    if (!shouldInternalize(*I, ExternalNames))
+    if (!maybeInternalize(*I, ExternalComdats))
       continue;
 
-    I->setVisibility(GlobalValue::DefaultVisibility);
-    I->setLinkage(GlobalValue::InternalLinkage);
-    Changed = true;
     ++NumGlobals;
     DEBUG(dbgs() << "Internalized gvar " << I->getName() << "\n");
   }
 
   // Mark all aliases that are not in the api as internal as well.
   for (Module::alias_iterator I = M.alias_begin(), E = M.alias_end();
        I != E; ++I) {
-    if (!shouldInternalize(*I, ExternalNames))
+    if (!maybeInternalize(*I, ExternalComdats))
       continue;
 
-    I->setVisibility(GlobalValue::DefaultVisibility);
-    I->setLinkage(GlobalValue::InternalLinkage);
-    Changed = true;
     ++NumAliases;
     DEBUG(dbgs() << "Internalized alias " << I->getName() << "\n");
   }
 
-  return Changed;
+  // We do not keep track of whether this pass changed the module because
+  // it adds unnecessary complexity:
+  // 1) This pass will generally be near the start of the pass pipeline, so
+  //    there will be no analyses to invalidate.
+  // 2) This pass will most likely end up changing the module and it isn't worth
+  //    worrying about optimizing the case where the module is unchanged.
+  return true;
 }
 
 ModulePass *llvm::createInternalizePass() { return new InternalizePass(); }

llvm/test/Transforms/Internalize/comdat.ll

@@ -0,0 +1,52 @@
+; RUN: opt < %s -internalize -internalize-public-api-list c1 -internalize-public-api-list c2 -internalize-public-api-list c3 -internalize-public-api-list c4 -S | FileCheck %s
+
+$c1 = comdat any
+$c2 = comdat any
+$c3 = comdat any
+$c4 = comdat any
+
+; CHECK: @c1_c = global i32 0, comdat($c1)
+@c1_c = global i32 0, comdat($c1)
+
+; CHECK: @c2_b = internal global i32 0{{$}}
+@c2_b = global i32 0, comdat($c2)
+
+; CHECK: @c3 = global i32 0, comdat{{$}}
+@c3 = global i32 0, comdat
+
+; CHECK: @c4_a = internal global i32 0, comdat($c4)
+@c4_a = internal global i32 0, comdat($c4)
+
+; CHECK: @c1_d = alias i32* @c1_c
+@c1_d = alias i32* @c1_c
+
+; CHECK: @c2_c = internal alias i32* @c2_b
+@c2_c = alias i32* @c2_b
+
+; CHECK: @c4 = alias i32* @c4_a
+@c4 = alias i32* @c4_a
+
+; CHECK: define void @c1() comdat {
+define void @c1() comdat {
+  ret void
+}
+
+; CHECK: define void @c1_a() comdat($c1) {
+define void @c1_a() comdat($c1) {
+  ret void
+}
+
+; CHECK: define internal void @c2() {
+define internal void @c2() comdat {
+  ret void
+}
+
+; CHECK: define internal void @c2_a() {
+define void @c2_a() comdat($c2) {
+  ret void
+}
+
+; CHECK: define void @c3_a() comdat($c3) {
+define void @c3_a() comdat($c3) {
+  ret void
+}