[patch] Don't let virtual calls and dynamic casts call MarkVTableUsed()

[Reid Kleckner]

I'm concerned that if you don't mark the vtable used enough, then codegen
will crash trying to emit a vtable for a class that it assumed would be
marked used because it's compiling a virtual call to a method of such a
class. However, it looks like Rafael completely nuked the
available_externally vtable emission optimization in r189852, which I
forgot about. The vtable code still has lots of rigging to allow
available_externally vtable emission, though.

So, I want to say that before r189852, these MarkVTableUsed calls were
necessary, and now that we don't do it, they are not. However, I'd really
like that optimization back, so we can optimize this code, once again:

struct A {
  virtual void f();
};
static void h(A &a) {
  a.f(); // requires inlining, must devirtualize in LLVM, requires
available_externally vtable
}
void g() {
  A a;
  a.f(); // obvious, devirtualized in frontend
  h(a);
}

On Tue, Jan 13, 2015 at 10:40 AM, Nico Weber <thakis at chromium.org> wrote:

> Hi,
>
> clang currently calls MarkVTableUsed() for classes that get their virtual
> methods called or that participate in a dynamic_cast. This is unnecessary,
> since CodeGen only emits vtables when it generates constructor, destructor,
> and vtt code.
>
> Remember that sema's MarkVTableUsed() doesn't actually cause emission of a
> vtable, it only marks all methods that are in the vtable as referenced – to
> make sure that when codegen decides to write a vtable, everything in there
> is actually defined. Since virtual calls and dynamic casts do not cause
> codegen to write a vtable, it's not necessary to define methods in the
> vtable for them. After this patch, sema only calls MarkVTableUsed() for the
> definitions of constructors and destructors – the only two things that make
> codegen write a vtable* (look for `getAddrOfVTable` in lib/CodeGen).
>
> They still get defined if they're referenced by a direct call – this only
> affects virtual methods that are not called and were only referenced by
> MarkVTableUsed(), for example if a different virtual method was called).
> For the same reason, this doesn't have an effect on what llvm can or can't
> optimize (see also PR20337 comment 10). It does lead to the emission of
> fewer unneeded methods, which will speed up builds.
>
> While this shouldn't change the behavior of codegen (other than being
> faster), it does make clang more permissive: virtual methods (in particular
> destructors) end up being instantiated less often. In particular, classes
> that have members that are smart pointers to incomplete types will now get
> their implicit virtual destructor instantiated less frequently. For
> example, this used to not compile but does now compile:
>
>     template <typename T> struct OwnPtr {
>       ~OwnPtr() { static_assert((sizeof(T) > 0), "TypeMustBeComplete"); }
>     };
>     class ScriptLoader;
>     struct Base { virtual ~Base(); };
>     struct Sub : public Base {
>       virtual void someFun() const {}
>       OwnPtr<ScriptLoader> m_loader;
>     };
>     void f(Sub *s) { s->someFun(); }
>
> The more permissive behavior matches both gcc (where this is not often
> observable, since in practice most things with virtual methods have a key
> function) and cl (which is my motivation for this change) – this fixes
> PR20337.
>
> This is similar to r213109 in spirit. r225761 was a prerequisite for this
> change.
>
> Various tests relied on "a->f()" marking a's vtable as used (in the sema
> sense), switch these to just construct a on the stack. This forces
> instantiation of the implicit constructor, which will mark the vtable as
> used.
>
> * The exception is -fapple-kext mode: In this mode, qualified calls to
> virtual functions (`a->Base::f()`) still go through the vtable, and since
> the vtable pointer off this doesn't point to Base's vtable, this needs to
> reference Base's vtable directly. To keep this working, keep referencing
> the vtable for virtual calls in apple kext mode. There's 0 test coverage
> for this :-/
>
> Nico
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150113/5c7a8be7/attachment.html>