Page MenuHomePhabricator

[modules] Defer emission of inline key functions.
Needs ReviewPublic

Authored by v.g.vassilev on Oct 31 2018, 3:47 AM.

Details

Summary

If the ABI supports inline key functions we must emit them eagerly. The generic reason is that CodeGen might assume they are emitted and generate a reference to the vtable. In deserialization this rule becomes a little less clear because if a reference is generated, CodeGen will make a request and we will emit the vtable on demand.

This patch improves the performance of loading modules by slimming down the EAGERLY_DESERIALIZED_DECLS sections. It is equivalent to 'pinning' the vtable, with two benefits: (a) it does not require expertise in modules to find out that outlining the key function brings performance; (b) it also helps with third-party codebases.

We ran this change through our codebase and we saw no observable changes in behavior.

Patch by Yuka Takahashi and me!

Diff Detail

Repository
rC Clang

Event Timeline

v.g.vassilev created this revision.Oct 31 2018, 3:47 AM

@rsmith, @bruno could you verify this patch on your codebases?

Seems reasonable to me.

If CodeGen is able to emit inline key functions when the vtable is needed anyway, can we instead change DeclMustBeEmitted to not treat inline key functions specially? There is no reason we need to emit any symbols for:

struct A { virtual void f(); };
inline void A::f() {}

... and if suppressing DeclMustBeEmitted works in the modules case, it really should work in the general case.

Indeed, deleting the relevant code from DeclMustBeEmitted appears to work fine. It's not actually enough to prevent us from spuriously emitting the vtable and inline function definition in the above case, though: Sema marks the vtable as "used" because the key function was defined (it doesn't know whether CodeGen might decide to emit it and prepares for the worst, but that then marks the vtable as "used", which *forces* it to be emitted). Fixing that requires some more invasive changes.

See https://reviews.llvm.org/D54986 for my proposed alternative to this. The idea is to allow CodeGen to decide when to emit a vtable, and to do so only when we actually emit a use of the vtable / key function / explicit instantiation definition of the class. This doesn't require any eagerly-deserialized declaration tracking except for explicit instantiation definitions.

lib/Serialization/ASTWriterDecl.cpp
2267–2278

If the function must be emitted for some reason other than being a key function (for instance, due to __attribute__((used)) or simply not being declared inline), this may result in it not getting emitted; that doesn't seem correct. (For a module it might typically work because most such cases would be redefinition errors, but it's wrong for cases like a precompiled preamble or PCH, which can be intended to be used only once and thus can contain strong external symbol definitions.)