[PATCH] D53234: Don't remove COMDATs when internalizing global objects

[Aaron Hill via Phabricator via llvm-commits]

Aaron1011 updated this revision to Diff 169735.
Aaron1011 added a comment.

> My reading of that commit (https://reviews.llvm.org/D10679), and looking at the code, is that GlobalDCE would be able to remove these members because they should *all* be removed from the comdat by internalization. The change appears to make it such that internalization cannot proceed if any member of the comdat is externally visible and therefore cannot be internalized. If all members of the comdat can be internalized (i.e. not in the ExternalComdats set), then they can all be internalized and the comdat removed from all members.

The issue only occurs when a pass like GlobalDCE runs //after// Internalizer. Since the COMDAT has been removed, GlobalDCE might remove only one member from the (now deleted) COMDAT, leaving other members intact.
I've added a testcase demonstrating this.


https://reviews.llvm.org/D53234

Files:
  lib/Transforms/IPO/Internalize.cpp
  test/Transforms/Internalize/comdat.ll
  test/Transforms/Internalize/preserve-comdat-members.ll


Index: test/Transforms/Internalize/preserve-comdat-members.ll
===================================================================
--- /dev/null
+++ test/Transforms/Internalize/preserve-comdat-members.ll
@@ -0,0 +1,19 @@
+; Running internalize followed by globaldce should preserve unused globals that share a COMDAT with used globals
+; RUN: opt < %s -internalize -internalize-public-api-list c1 -globaldce  -S | FileCheck %s
+
+$c1 = comdat any
+
+; Since both used_global and and unused_fn share a COMDAT, they should both be kept,
+; keeping the !associated metadata pointing at unused_fn
+; CHECK: @used_global = internal global i32 0, comdat($c1), !associated !0
+ at used_global = global i32 0, comdat($c1), !associated !0
+
+ at llvm.used = appending global [1 x i32*] [i32* @used_global], section "llvm.metadata"
+
+; CHECK: define internal void @unused_fn() comdat($c1) {
+define void @unused_fn() comdat($c1) {
+  ret void
+}
+
+; CHECK: !0 = !{void ()* @unused_fn}
+!0 = !{void ()* @unused_fn}
\ No newline at end of file
Index: test/Transforms/Internalize/comdat.ll
===================================================================
--- test/Transforms/Internalize/comdat.ll
+++ test/Transforms/Internalize/comdat.ll
@@ -8,7 +8,7 @@
 ; CHECK: @c1_c = global i32 0, comdat($c1)
 @c1_c = global i32 0, comdat($c1)
 
-; CHECK: @c2_b = internal global i32 0{{$}}
+; CHECK: @c2_b = internal global i32 0, comdat($c2)
 @c2_b = global i32 0, comdat($c2)
 
 ; CHECK: @c3 = global i32 0, comdat{{$}}
@@ -36,12 +36,12 @@
   ret void
 }
 
-; CHECK: define internal void @c2() {
+; CHECK: define internal void @c2() comdat {
 define internal void @c2() comdat {
   ret void
 }
 
-; CHECK: define internal void @c2_a() {
+; CHECK: define internal void @c2_a() comdat($c2) {
 define void @c2_a() comdat($c2) {
   ret void
 }
Index: lib/Transforms/IPO/Internalize.cpp
===================================================================
--- lib/Transforms/IPO/Internalize.cpp
+++ lib/Transforms/IPO/Internalize.cpp
@@ -119,10 +119,6 @@
     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 {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D53234.169735.patch
Type: text/x-patch
Size: 2292 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181015/11557c44/attachment.bin>