[ThinLTO] Copy UnnamedAddr when spliting module.

[zequanwu]

Summary

The unnamedaddr property of a function is lost when using
-fwhole-program-vtables and thinlto which causes size increase under linker's
safe icf mode.

Chrome built with safe-icf on Linux and Windows reduced 2Mb binary size after
this change.

There is a repro:

# a.h
struct A {
  virtual int f();
  virtual int g();
};

# a.cpp
#include "a.h"
int A::f() { return 10; }
int A::g() { return 10; }

# main.cpp
#include "a.h"

int g(A* a) {
  return a->f();
}

int main(int argv, char** args) {
  A a;
  return g(&a);
}

$ clang++ -O2 -ffunction-sections -flto=thin -fwhole-program-vtables -fsplit-lto-unit -c main.cpp -o main.o  && clang++ -O2 -ffunction-sections -flto=thin -fwhole-program-vtables -fsplit-lto-unit -c a.cpp -o a.o && clang++ -Wl,--icf=safe -fuse-ld=lld  -flto=thin a.o main.o -o a.out && llvm-readobj -t a.out | grep -A 1 -e _ZN1A1fEv -e _ZN1A1gEv
    Name: _ZN1A1fEv (480)
    Value: 0x201830
--
    Name: _ZN1A1gEv (490)
    Value: 0x201840

[rnk]

Please add a test.

I know you said that copyAttributesFrom crashes, but there are many other attributes to consider:
https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/Globals.cpp#L65

void GlobalValue::copyAttributesFrom(const GlobalValue *Src) {
  setVisibility(Src->getVisibility());
  setUnnamedAddr(Src->getUnnamedAddr());
  setThreadLocalMode(Src->getThreadLocalMode());
  setDLLStorageClass(Src->getDLLStorageClass());
  setDSOLocal(Src->isDSOLocal());
  setPartition(Src->getPartition());
}
...
void GlobalObject::copyAttributesFrom(const GlobalObject *Src) {
  GlobalValue::copyAttributesFrom(Src);
  setAlignment(MaybeAlign(Src->getAlignment()));
  setSection(Src->getSection());
}

Some of these, like DSO local or the alignment, might be worth copying. I think I would prefer to figure out which of these attributes is causing the crash, to clear that attribute from F (we are about to erase F), and then to copy the rest of the attributes. I suspect there is some assertion about function attributes that cannot be applied to a declaration, or that the function has argument attributes for parameters that are not part of the void prototype. If it ends up being the AttributeList that causes the crash, you can clear them all or copy them more selectively.

[zequanwu]

This comment was removed by zequanwu.

[zequanwu]

• Copy function attributes and remove ret/param attributes.
• Add a test case.

[rnk]

Looks good with a minor simplification

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

163

Instead of iterating over return and parameter attributes, I suggest you build a new AttributeList that consists of nothing but function attributes. This code might work, but I haven't actually compiled it:

NewF->setAttributes(AttributeList::get(M.getContext(), {AttributeList::FunctionIndex, F.getAttributes().getFnAttributes()}));

This has a nice side effect of not leaking lots of temporary AttributeList objects.

[zequanwu]

update and tested to build chrome.

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

for (auto I = M.begin(), E = M.end(); I != E;) {
     if (!F.isDeclaration() || F.getFunctionType() == EmptyFT ||
         // Changing the type of an intrinsic may invalidate the IR.
         F.getName().startswith("llvm."))
       continue;
 
     Function *NewF =
         Function::Create(EmptyFT, GlobalValue::ExternalLinkage,
                          F.getAddressSpace(), "", &M);
-    NewF->setVisibility(F.getVisibility());
+    // Only copy function attribtues.
+    NewF->copyAttributesFrom(&F);
+    auto Attrs = F.getAttributes();
+    for (unsigned i = 0; i < F.arg_size(); ++i) {
+      NewF->removeParamAttrs(i, Attrs.getParamAttributes(i));
+    }
+    NewF->removeAttributes(AttributeList::ReturnIndex,
+                           Attrs.getRetAttributes());
     NewF->takeName(&F);
     F.replaceAllUsesWith(ConstantExpr::getBitCast(NewF, F.getType()));
     F.eraseFromParent();
   }
 
   for (auto I = M.global_begin(), E = M.global_end(); I != E;) {
     GlobalVariable &GV = *I++;
     if (GV.isDeclaration() && GV.use_empty()) {

llvm/test/Transforms/ThinLTOBitcodeWriter/split-vfunc.ll

 ; RUN: opt -thinlto-bc -thinlto-split-lto-unit -o %t %s
 ; RUN: llvm-modextract -b -n 0 -o - %t | llvm-dis | FileCheck --check-prefix=M0 %s
 ; RUN: llvm-modextract -b -n 1 -o - %t | llvm-dis | FileCheck --check-prefix=M1 %s
 
-; M0: @g = external constant [9 x i8*]{{$}}
-; M1: @g = constant [9 x i8*]
-@g = constant [9 x i8*] [
+; M0: @g = external constant [10 x i8*]{{$}}
+; M1: @g = constant [10 x i8*]
+@g = constant [10 x i8*] [
   i8* bitcast (i64 (i8*)* @ok1 to i8*),
   i8* bitcast (i64 (i8*, i64)* @ok2 to i8*),
   i8* bitcast (void (i8*)* @wrongtype1 to i8*),
   i8* bitcast (i128 (i8*)* @wrongtype2 to i8*),
   i8* bitcast (i64 ()* @wrongtype3 to i8*),
   i8* bitcast (i64 (i8*, i8*)* @wrongtype4 to i8*),
   i8* bitcast (i64 (i8*, i128)* @wrongtype5 to i8*),
   i8* bitcast (i64 (i8*)* @usesthis to i8*),
-  i8* bitcast (i8 (i8*)* @reads to i8*)
+  i8* bitcast (i8 (i8*)* @reads to i8*),
+  i8* bitcast (i8* (i8*, i8)* @attributedFunc to i8*)
 ], !type !0
 
 ; M0: define i64 @ok1
 ; M1: define available_externally i64 @ok1
 define i64 @ok1(i8* %this) {
   ret i64 42
 }
 

 
 ; M0: define i8 @reads
 ; M1: declare void @reads()
 define i8 @reads(i8* %this) {
   %l = load i8, i8* %this
   ret i8 %l
 }
 
+; Check function attributes are copied over splitted module
+; M0: declare dso_local noundef i8* @attributedFunc(i8* noalias, i8 zeroext) unnamed_addr #[[ATTR0:[0-9]+]]
+; M1: declare dso_local void @attributedFunc() unnamed_addr #[[ATTR1:[0-9]+]]
+declare dso_local noundef i8* @attributedFunc(i8* noalias, i8 zeroext) unnamed_addr alwaysinline willreturn
+; M0: attributes #[[ATTR0]] = { alwaysinline willreturn }
+; M1: attributes #[[ATTR1]] = { alwaysinline willreturn }
+
 !0 = !{i32 0, !"typeid"}