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

[ben via Phabricator via llvm-commits]

bd1976llvm added a comment.

In D53234#1310254 <https://reviews.llvm.org/D53234#1310254>, @Aaron1011 wrote:

> > 2. Retain the comdats in the output (this patch). Limits the optimization potential of LTO. !associated metadata sections don't need to be in a comdats.
>
> I don't believe that this is the right way to think about this. This patch prevents LLVM from performing an //invalid// optimization - that is, selectively discarding global objects that the IR explicitly indicates should stay together.


We need to resolve this point of disagreement. IMO that optimization is only invalid in regular compilation. I think that for LTO this is a valid optimization. LTO has optimizations that would normally be invalid - for example the Internalize pass.

>> Add GV's with !associated metadata to llvm.compiler.used.
>> 
>>   Limits the optimization potential of LTO (but not as much as 2.).
> 
> LLVM already does this in SanitizerCoverage <https://github.com/llvm-mirror/llvm/blob/e4c346daccc176632a9c10e88a424d126efd0602/lib/Transforms/Instrumentation/SanitizerCoverage.cpp#L588>
> 
> Did you mean adding the target of the !associated metadata to  llvm.compiler.used? If so, I don't believe that there's a difference between this option and option 2.

Yes, I mean't adding the target of the !associated metadata to llvm.compiler.used.

> Consider the case of a function that's unused, except for the fact that it's targeted by a global via !associated metadata. If both share a COMDAT, LTO will still be able to remove them from the final object,
>  since neither is considered used.

I don't think this will happen because the !associated metadata section will always be referenced from llvm.compiler.used which will prevent the COMDAT from being removed, no?
AFAIK !associated metadata sections are not required to be in a COMDAT; so, option 3. is a more general solution.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53234/new/

https://reviews.llvm.org/D53234