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

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

rsmith marked an inline comment as done.
rsmith added a comment.

In D54986#1312435 <https://reviews.llvm.org/D54986#1312435>, @lebedev.ri wrote:

> Will `-fforce-emit-vtables` still work?


Yes, it still works. (The tests for it still pass and I've manually tested it too.)



================
Comment at: lib/AST/ASTContext.cpp:9801
+           RD->getTemplateSpecializationKind() ==
+               TSK_ExplicitInstantiationDefinition;
   else
----------------
rjmccall wrote:
> Does it matter if it's not this particular declaration that's the explicit instantiation?
The explicit instantiation definition decl itself is the only one that CodeGen needs to see. (Earlier declarations would have a different TSK, and the language rules generally don't permit any later decls.)


================
Comment at: lib/AST/RecordLayoutBuilder.cpp:3069
   const Decl *Result =
       Entry ? Entry.get(getExternalSource()) : computeKeyFunction(*this, RD);
 
----------------
rjmccall wrote:
> Why not just change `computeKeyFunction` to decide that the class doesn't have a key function if it would be an inline definition?  We can do the same updating we do with the ARM rule except just bailing out instead of scanning for another candidate.
It seems desirable to me for Clang's notion of what the "key function" is to exactly match the ABI specification. That way, the decision to not emit a vtable with an inline key function is purely a lowering concern, not part of the program model represented by the frontend, and non-compiler tools who query this data will get correct answers. Also, if we made the suggested change, I'd still want to perform the linkage check to avoid unnecessary emission of vtables for explicit instantiation definitions, so this wouldn't remove any complexity from CodeGen.

If we want to go down this path, I'd prefer that we also change the ABI document to say that an inline virtual function definition results in the class having no key function.


================
Comment at: lib/CodeGen/CGVTables.cpp:889
+  if (!llvm::GlobalValue::isDiscardableIfUnused(getVTableLinkage(theClass)))
+    getCXXABI().emitVTables(theClass);
 }
----------------
rjmccall wrote:
> I feel like "don't emit the v-table for this" should be handled in the caller.
OK, but that'd mean putting the same check into both callers of this function. As an alternative, what do you think about renaming this function to something more like "MarkVTableDemanded" that doesn't suggest that we'll necessarily emit it?


Repository:
  rC Clang

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

https://reviews.llvm.org/D54986