[ThinLTO] Don't internalize during promotion

[evgeny777]

Summary

This can raise problems when internalised symbol is in comdat, while another member of comdat is exported. Such case is handled by internalize pass which is invoked later.

[steven_wu]

I think this change is generally good but sorry I am not quite familiar with the semantics of comdat. Can you explain in more detail why that can be a problem to internalize that in promote?

[evgeny777]

From https://llvm.org/docs/LangRef.html#comdats :

Comdats have a name which represents the COMDAT key. All global objects that specify this key will only end up in the final object file if the linker chooses that key over some other key

To enforce this LLVM internalize pass doesn't internalize entire COMDAT group if any of its members should be preserved (see shouldPreserveGV). At the moment we can internalize global
in llvm::thinLTOResolvePrevailingInModule without looking at other COMDAT group members and break this guarantee

[steven_wu]

In D69107#1714175, @evgeny777 wrote:

From https://llvm.org/docs/LangRef.html#comdats :

Comdats have a name which represents the COMDAT key. All global objects that specify this key will only end up in the final object file if the linker chooses that key over some other key

To enforce this LLVM internalize pass doesn't internalize entire COMDAT group if any of its members should be preserved (see shouldPreserveGV). At the moment we can internalize global
in llvm::thinLTOResolvePrevailingInModule without looking at other COMDAT group members and break this guarantee

Can you add a testcase which represents the situation?

[evgeny777]

Added test case

[steven_wu]

LGTM. Thanks!

[evgeny777]

@tejohnson Teresa, are you ok with it?

[tejohnson]

In D69107#1716983, @evgeny777 wrote:

@tejohnson Teresa, are you ok with it?

Yep, just had a chance to look. Suggest adding a comment above the new check that internalization is done separately since the internalization code contains the necessary correctness checks.

lib/Transforms/IPO/FunctionImport.cpp

auto updateLinkage = [&](GlobalValue &GV) {
     // See if the global summary analysis computed a new resolved linkage.
     const auto &GS = DefinedGlobals.find(GV.getGUID());
     if (GS == DefinedGlobals.end())
       return;
     auto NewLinkage = GS->second->linkage();
     if (NewLinkage == GV.getLinkage())
       return;
     if (GlobalValue::isLocalLinkage(GV.getLinkage()) ||
+        GlobalValue::isLocalLinkage(NewLinkage) ||
         // In case it was dead and already converted to declaration.
         GV.isDeclaration())
       return;
 
     // Check for a non-prevailing def that has interposable linkage
     // (e.g. non-odr weak or linkonce). In that case we can't simply
     // convert to available_externally, since it would lose the
     // interposable property and possibly get inlined. Simply drop

test/ThinLTO/X86/weak_externals.ll

 ; Test that linkonce_odr and weak_odr variables which are visible to regular
 ; object (and so are not readonly) are not internalized by thin LTO.
 ; RUN: opt -module-summary %s -o %t.bc
 ; RUN: llvm-lto2 run -save-temps %t.bc -o %t.out \
 ; RUN:               -r=%t.bc,_ZL5initSv,plx \
 ; RUN:               -r=%t.bc,_ZN9SingletonI1SE11getInstanceEv,lx \
 ; RUN:               -r=%t.bc,_ZZN9SingletonI1SE11getInstanceEvE8instance,lx \
 ; RUN:               -r=%t.bc,_ZZN9SingletonI1SE11getInstanceEvE13instance_weak,lx
 ; RUN: llvm-dis %t.out.1.1.promote.bc -o - | FileCheck %s
+; RUN: llvm-dis %t.out.1.2.internalize.bc -o - | FileCheck %s --check-prefix=INTERNALIZE
 
 ; CHECK: @_ZZN9SingletonI1SE11getInstanceEvE8instance = available_externally dso_local global %struct.S zeroinitializer
 ; CHECK: @_ZZN9SingletonI1SE11getInstanceEvE13instance_weak = available_externally dso_local global %struct.S* null, align 8
-; CHECK: define internal dereferenceable(16) %struct.S* @_ZN9SingletonI1SE11getInstanceEv() comdat
+; CHECK: define linkonce_odr dso_local dereferenceable(16) %struct.S* @_ZN9SingletonI1SE11getInstanceEv() comdat
+; INTERNALIZE: define internal dereferenceable(16) %struct.S* @_ZN9SingletonI1SE11getInstanceEv()
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 %struct.S = type { i64, i64 }
 
 $_ZN9SingletonI1SE11getInstanceEv = comdat any