<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jan 13, 2015 at 11:11 AM, Reid Kleckner <span dir="ltr"><<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div>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.</div></div></div></blockquote><div><br></div><div>At the moment, the only thing in coding adding to the DeferredVTables vector in codegen is getAddrOfVTable(), and that's only called for structor body emission (and apple kext vcalls). So I think this should be fine.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><br></div><div>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:</div><div><br></div><div><div>struct A {</div><div> virtual void f();</div><div>};</div><div>static void h(A &a) {</div><div> a.f(); // requires inlining, must devirtualize in LLVM, requires available_externally vtable</div><div>}</div><div>void g() {</div><div> A a;</div><div> a.f(); // obvious, devirtualized in frontend</div><div> h(a);</div><div>}</div></div></div></blockquote><div><br></div><div>This is discussed in <a href="http://llvm.org/bugs/show_bug.cgi?id=20337#c18">http://llvm.org/bugs/show_bug.cgi?id=20337#c18</a> (sorry, got the comment number wrong above). In this case, we're still able to optimize (in theory) since A has an implicit constructor that gets emitted which does cause emission of a vtable. If you make the constructor explicit and out of line (as in that bug comment), that's no longer true – but if we end up implementing that at some point, we can worry about it then.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div></div></div><div class=""><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 13, 2015 at 10:40 AM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Hi,<div><br></div><div>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.</div><div><br></div><div>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).</div>
<div><br></div><div>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.</div><div><br></div><div>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:</div><div><br></div><div><div> template <typename T> struct OwnPtr {</div><div> ~OwnPtr() { static_assert((sizeof(T) > 0), "TypeMustBeComplete"); }</div><div> };</div><div> class ScriptLoader;<br></div><div> struct Base { virtual ~Base(); };<br></div><div> struct Sub : public Base {</div><div> virtual void someFun() const {}</div><div> OwnPtr<ScriptLoader> m_loader;</div><div> };</div><div> void f(Sub *s) { s->someFun(); }<br></div></div><div><br></div><div>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.</div><div><br></div><div>This is similar to r213109 in spirit. r225761 was a prerequisite for this change.</div><div><br>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.</div><div><br></div><div>* 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 :-/</div><span><font color="#888888"><div><br></div><div>Nico</div></font></span></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>