[PATCH] D54986: Make CodeGen choose when to emit vtables.

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

rsmith marked 2 inline comments as done.
rsmith added inline comments.


================
Comment at: lib/CodeGen/CodeGenModule.cpp:3991-3992
+  if (auto *MD = dyn_cast<CXXMethodDecl>(D)) {
+    // FIXME: There's no reason to do this if the key function is inline.
+    // Formally, the ABI requires it, but the difference is not observable.
+    if (declaresSameEntity(Context.getCurrentKeyFunction(MD->getParent()), MD))
----------------
rjmccall wrote:
> rsmith wrote:
> > rjmccall wrote:
> > > rsmith wrote:
> > > > @rjmccall Is there any reason we (from the CodeGen perspective) should treat an inline key function as emitting the vtable? I can't think of any reason to do so -- it's not in a comdat with the vtable or anything like that, so every translation unit that emits a reference to the vtable should emit its own copy anyway.
> > > The thinking was as follows: translation units that can't see a definition of the key function don't know that the definition is actually inline, so they'll emit a reference to an external v-table definition, which will lead to link failures if the translation units that do contain the inline definition don't eagerly emit the v-table.  However, ARM pointed out years ago that the ODR requires inline definitions of virtual functions to be present in every translation unit which declares the virtual function at all, so there's no legal situation where a translation unit can't see the definition of an inline key function.  Furthermore, I believe Clang has never made v-tables undiscardable in translation units with inline key function definitions, so there's no real guarantee that ODR-violating code will actually link, although you can certainly imagine ways in which an ODR-violating program might get by without such a guarantee.
> > > 
> > > Personally, I think the strongest argument for "deviating" here is that the language standard takes priority over the ABI, which means we're allowed to assume the program is overall well-formed, i.e. we're only required to interoperate with legal code.  Now, that's a line of reasoning which leads us into some grey areas of implementation-definedness, but I feel fairly comfortable about deviating in this particular instance because I don't know why someone would really *want* to take advantage of v-tables being emitted eagerly; in general, eager emission of inline code is a bad thing that significantly slows down builds.
> > > 
> > > Now, ARM used this property of inline definitions to change the key function to the first non-inline function, and unfortunately we can't do that on existing targets without breaking interoperation.  (We did do it on some newer Darwin targets, though, and we haven't had any problem with it.)  But I do think we could use this property of inline definitions to just treat the class as no longer having a key function when we see an inline definition of it.  That would rid us of this particular scourge of eager emission of inline code.
> > My thinking is this: if a vtable has discardable linkage, then it can be discarded when optimizing if it's not referenced. So there's no point emitting it unless we also emit a reference to it. So we should only emit vtables with discardable linkage if they're actually referenced, just like we do for other symbols with discardable linkage. This is, I think, stronger than what you're suggesting, because it affects internal-linkage explicit instantiations too.
> Given only the ABI rule, using discardable linkage is a bug.  If you take the "those translation units containing the definition must emit the v-table so that other translation units can use it" argument seriously, you obviously can't use discardable linkage, because the other translation units can't use it.  That's why I bothered developing the whole argument above about why it's okay to ignore the ABI rule here.
> 
> Your argument about internal-linkage explicit instantiations abstractly makes a lot of sense but also sets off a bunch of klaxons in my mind about ignoring obvious programmer intent.  Still, I think it's reasonable to try it out.
OK, fair enough. I was only starting from the "these vtables have discardable linkage" position because that has been the status quo in Clang ~forever (godbolt.org only goes back to Clang 3). I'll give it a go and see what shakes out.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54986