[Inliner] Only remove functions with a COMDAT when it's safe to do so

[Aaron1011]

Summary

Currently, Inliner will sometimes delete functions in a COMDAT without checking to see if the COMDAT has any other members.
As described in https://reviews.llvm.org/D53234, this is an invalid treatment of COMDAT members, and can lead to issues when using MD_Associated metadata.

I've modified Inliner to ensure that deletion of inlined functions in a COMDAT is always delayed until 'removeDeadFunctions', where the proper checks are performed.
Additionally, I've modified 'removeDeadFunctions' to always process functions with a COMDAT via 'filterDeadComdatFunctions', regardless of their linkage.

I've added a test case to check that the presence of other members in a COMDAT prevents Inliner from deleting a function.

While adding some extra LLVM_DEBUG prints, I found and fixed a bug with printing unnamed COMDATs. It's a one-line fix, so I didn't think it was worth moving into its own patch.

[Aaron1011]

Ping

[bd1976llvm]

Hi Aron, I'm interested in reviewing this. Would you mind responding to my comments on https://reviews.llvm.org/D53234 first, as you have based the correctness of this on the correctness of that change?

lib/IR/AsmWriter.cpp

void NamedMDNode::print(raw_ostream &ROS, ModuleSlotTracker &MST,
   }
 
   formatted_raw_ostream OS(ROS);
   AssemblyWriter W(OS, *SlotTable, getParent(), nullptr, IsForDebug);
   W.printNamedMDNode(this);
 }
 
 void Comdat::print(raw_ostream &ROS, bool /*IsForDebug*/) const {
-  PrintLLVMName(ROS, getName(), ComdatPrefix);
+  PrintLLVMName(ROS, Name != nullptr ? getName() : "<unnamed>", ComdatPrefix);
   ROS << " = comdat ";
 
   switch (getSelectionKind()) {
   case Comdat::Any:
     ROS << "any";
     break;
   case Comdat::ExactMatch:
     ROS << "exactmatch";

lib/Transforms/IPO/Inliner.cpp

for (unsigned CSi = 0; CSi != CallSites.size(); ++CSi) {
       // If we inlined or deleted the last possible call site to the function,
       // delete the function body now.
       if (Callee && Callee->use_empty() && Callee->hasLocalLinkage() &&
           // TODO: Can remove if in SCC now.
           !SCCFunctions.count(Callee) &&
           // The function may be apparently dead, but if there are indirect
           // callgraph references to the node, we cannot delete it yet, this
           // could invalidate the CGSCC iterator.
-          CG[Callee]->getNumReferences() == 0) {
+          CG[Callee]->getNumReferences() == 0 &&
+          // Functions in a COMDAT require special handling
+          // to ensure that we always drop either all or none
+          // of its members. This is handled by 'removeDeadFunctions',
+          // which will be called later.
+          !Callee->hasComdat()) {
         LLVM_DEBUG(dbgs() << "    -> Deleting dead function: "
                           << Callee->getName() << "\n");
         CallGraphNode *CalleeNode = CG[Callee];
 
         // Remove any call graph edges from the callee to its callees.
         CalleeNode->removeAllCalledFunctions();
 
         // Removing the node for callee from the call graph and delete it.
for (const auto &I : CG) {
     // them.
     F->removeDeadConstantUsers();
 
     if (!F->isDefTriviallyDead())
       continue;
 
     // It is unsafe to drop a function with discardable linkage from a COMDAT
     // without also dropping the other members of the COMDAT.
-    // The inliner doesn't visit non-function entities which are in COMDAT
-    // groups so it is unsafe to do so *unless* the linkage is local.
-    if (!F->hasLocalLinkage()) {
-      if (F->hasComdat()) {
-        DeadFunctionsInComdats.push_back(F);
-        continue;
-      }
-    }
+    // If the function has a comdat, we delay removing it
+    // so that we can check to see if we're going to drop the other COMDAT
+    // members.
+    LLVM_DEBUG(dbgs() << "Preparing to delete function: " << F->getName());
+      // Printing a COMDAT also prints a newline
+      LLVM_DEBUG(dbgs() << " with COMDAT: " << *F->getComdat());
+    LLVM_DEBUG(dbgs() << "\n");
 
     RemoveCGN(CGN);
   }
   if (!DeadFunctionsInComdats.empty()) {
     // Filter out the functions whose comdats remain alive.
     filterDeadComdatFunctions(CG.getModule(), DeadFunctionsInComdats);
     // Remove the rest.
     for (Function *F : DeadFunctionsInComdats)

test/Transforms/Inline/comdat-ipo.ll

 ; RUN: opt -inline -S < %s | FileCheck %s
 ; RUN: opt -passes='cgscc(inline)' -S < %s | FileCheck %s
 
+$c = comdat any
+
+; CHECK: @comdat_global = global i32 0, comdat($c)
+@comdat_global = global i32 0, comdat($c)
+
+; CHECK: define internal void @comdat_function() comdat($c) {
+define internal void @comdat_function() comdat($c) {
+  ret void
+}
+
 define i32 @caller() {
 ; CHECK-LABEL: @caller(
 ; CHECK-NEXT:  %val2 = call i32 @linkonce_callee(i32 42)
 ; CHECK-NEXT:  ret i32 %val2
 
   %val = call i32 @odr_callee()
   %val2 = call i32 @linkonce_callee(i32 %val);
   ret i32 %val2