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

[Aaron Hill via Phabricator via llvm-commits]

Aaron1011 created this revision.
Aaron1011 added reviewers: pcc, tejohnson.
Herald added subscribers: llvm-commits, eraman.

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.


Repository:
  rL LLVM

https://reviews.llvm.org/D53242

Files:
  lib/IR/AsmWriter.cpp
  lib/Transforms/IPO/Inliner.cpp
  test/Transforms/Inline/comdat-ipo.ll


Index: test/Transforms/Inline/comdat-ipo.ll
===================================================================
--- test/Transforms/Inline/comdat-ipo.ll
+++ test/Transforms/Inline/comdat-ipo.ll
@@ -1,6 +1,16 @@
 ; 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)
+ at 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)
Index: lib/Transforms/IPO/Inliner.cpp
===================================================================
--- lib/Transforms/IPO/Inliner.cpp
+++ lib/Transforms/IPO/Inliner.cpp
@@ -710,7 +710,12 @@
           // 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];
@@ -807,14 +812,17 @@
 
     // 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());
+    if (F->hasComdat()) {
+      // Printing a COMDAT also prints a newline
+      LLVM_DEBUG(dbgs() << " with COMDAT: " << *F->getComdat());
+      DeadFunctionsInComdats.push_back(F);
+      continue;
     }
+    LLVM_DEBUG(dbgs() << "\n");
 
     RemoveCGN(CGN);
   }
Index: lib/IR/AsmWriter.cpp
===================================================================
--- lib/IR/AsmWriter.cpp
+++ lib/IR/AsmWriter.cpp
@@ -4067,7 +4067,7 @@
 }
 
 void Comdat::print(raw_ostream &ROS, bool /*IsForDebug*/) const {
-  PrintLLVMName(ROS, getName(), ComdatPrefix);
+  PrintLLVMName(ROS, Name != nullptr ? getName() : "<unnamed>", ComdatPrefix);
   ROS << " = comdat ";
 
   switch (getSelectionKind()) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D53242.169542.patch
Type: text/x-patch
Size: 2997 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181013/b8733721/attachment-0001.bin>