[PATCH] D53925: [modules] Defer emission of inline key functions.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

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.



================
Comment at: lib/Serialization/ASTWriterDecl.cpp:2267-2278
+  // 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 change in behavior is driven mostly by performance considerations and
----------------
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.)


Repository:
  rC Clang

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

https://reviews.llvm.org/D53925