Page MenuHomePhabricator

[ThinLTO] Copy UnnamedAddr when spliting module.
ClosedPublic

Authored by zequanwu on Apr 14 2021, 11:27 AM.

Details

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

Diff Detail

Event Timeline

zequanwu created this revision.Apr 14 2021, 11:27 AM
zequanwu requested review of this revision.Apr 14 2021, 11:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2021, 11:27 AM
zequanwu edited the summary of this revision. (Show Details)Apr 14 2021, 11:30 AM
rnk added a comment.Apr 14 2021, 11:41 AM

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.

This comment was removed by zequanwu.
zequanwu updated this revision to Diff 337600.Apr 14 2021, 6:29 PM
  • Copy function attributes and remove ret/param attributes.
  • Add a test case.
rnk accepted this revision.Apr 19 2021, 11:35 AM

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.

This revision is now accepted and ready to land.Apr 19 2021, 11:35 AM
zequanwu updated this revision to Diff 338634.Apr 19 2021, 2:03 PM

update and tested to build chrome.

This revision was landed with ongoing or failed builds.Apr 19 2021, 2:05 PM
This revision was automatically updated to reflect the committed changes.