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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D53234#1265743, @Aaron1011 wrote:

> > 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.


Normally once they are both internalized and removed from the comdat, it shouldn't matter if only one is kept by GlobalDCE. The other is simply an internal function and no longer part of a comdat (so it's not like the comdat is incomplete). Is the issue here that there is a dangling reference or undef from the !0 metadata after GlobalDCE? I'm not familiar with the associated metadata type, but shouldn't this metadata reference be preventing GlobalDCE from removing unused_fn? Would it be correct if they were both removed from the comdat but kept by GlobalDCE?

Hopefully @pcc will comment, since this was his change. I am not sure preventing the comdat removal in LTO is correct or desired.


https://reviews.llvm.org/D53234