[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

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

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

In D154658#4481225 <https://reviews.llvm.org/D154658#4481225>, @rjmccall wrote:

> In D154658#4479213 <https://reviews.llvm.org/D154658#4479213>, @rsmith wrote:
>
>> I think (hope?) we should be able to apply this to a much larger set of cases. Would it be correct to do this optimization unless the vtable might be emitted with vague linkage and non-default visibility (that is, unless we're in the odd case where people expect non-default visibility classes to be the same type across DSOs)? Or are there cases where we might be using a vtable that (eg) doesn't even have the right symbol?
>
> I don't know of any problems other than the total failure of vague linkage across DSO boundaries, so if we just treat that as an implicit exception to the vtable uniqueness guarantee in the ABI, I think you've got the condition exactly right: we could do this for any class where either the v-table doesn't have vague linkage or the class's formal visibility is not `default`.  Our experience at Apple with enforcing type visibility is that it's usually good for one or two bug reports a year, but it's pretty easy to explain to users what they did wrong, and Clang's visibility attributes are pretty simple to use.  (`type_visibility` helps a lot with managing tradeoffs.)

I did some checking through the Clang implementation and found another two cases:

- under `-fapple-kext`, vague-linkage vtables get emitted in each translation unit that references them
- under `-fno-rtti`, identical vtables for distinct types could get merged because we emit vtables as `unnamed_addr` (this doesn't affect `dynamic_cast`, because `-fno-rtti` also disables `dynamic_cast` entirely)

The good news seems to be that if you don't use any language extensions (type visibility, `-fno-rtti`, `-fapple-kext`), then we do actually provide the guarantee that the ABI describes. :)

In D154658#4479170 <https://reviews.llvm.org/D154658#4479170>, @rjmccall wrote:

> If there are multiple subobjects of the source type in the destination type, consider just casting to `void*` first instead of doing multiple comparisons.

This turned out to be a little subtle: the vptr comparison we do after the cast requires loading a vptr of an object of entirely-unknown type. This is the first time we're doing that in IR generation, and `GetVTablePtr` expected to be told which class it's loading the vtable for (presumably, so that it can load from the right offset within the class). However, the implementation of `GetVTablePtr` didn't use the class for anything; it's always at the start for all of our supported ABIs. But this patch would make it harder to support an ABI that didn't put the vptr at the start of the most-derived object.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154658