<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Aug 7, 2015, at 12:32 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Fri, Aug 7, 2015 at 11:30 AM, Pete Cooper <span dir="ltr" class=""><<a href="mailto:peter_cooper@apple.com" target="_blank" class="">peter_cooper@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="">I’ve almost finished a patch to add back in either out of line destructors or anchor methods. We seem to use one or the one, relatively inconsistently.<div class=""><br class=""></div><div class="">What i’ve gone for is that if a class already had an inline destructor then i left it alone and added an anchor method. Otherwise I added an out of line destructor.</div></div></blockquote><div class=""><br class="">That seems possibly the opposite of what might be desired - if the class has no user-declared dtor, it's implicitly inline and possibly trivial (& doesn't suppress implicit move and copy ops (rule of 0/5)) - I'd avoid adding a dtor to a class that doesn't have/need one (& whenever I see empty-bodied dtors I try to remove them - not always possible).<br class=""><br class="">I was probably the first one to use (or at least do a lot of them) the explicit anchors in an attempt to make LLVM -Wweak-vtables clean but never got around to going all the way there. I think Chris Lattner even advocated for not out-of-lining dtors just to make the vtables strong, instead using an explicit anchor. But I'm pretty ambivalent about it. In theory the style guide advocates this for build perf reasons - which would be interesting to test & see if it's actually relevant. If it's a matter of correctness/functionality as this thread seems to imply (but I haven't read it enough to really understand what's being dealt with here, to be honest) then I guess we'll have to decide on some approach... *shrug*<br class=""></div></div></div></div></div></blockquote>Thanks for the explanation. I knew we wouldn’t be able to inline the destructors when they are out of line, but i didn’t know about the rule of 0/5 which is also interesting.</div><div><br class=""></div><div>So I didn’t really like the look of what I did either which is why i thought it best to explain it. I’m happy to change over what I’ve done to anchor’s, but its unfortunate that it will then differ from some of the other sibling classes.</div><div><br class=""></div><div>Perhaps this is moot* though, if I can get back to finishing the devirtualisation without any perf issues.</div><div><br class=""></div><div>* moot for ToT perhaps. I’m happy to get something for 3.7 if we can work out what it should be. The options seem to be</div><div>- Do nothing to the code, except perhaps change llvm-config as David said</div><div>- This patch</div><div>- The equivalent to this with anchor’s.</div><div><br class=""></div><div>Anyone got a strong opinion which is best, especially as I don’t want to do anything too big for 3.7 at this point.</div><div><br class=""></div><div>Cheers,</div><div>Pete<br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class="">- Dave<br class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><br class=""></div><div class="">Now if I compile Instructions.cpp with -Wweak-vtable, the only warnings given are:</div><div class=""><br class=""></div><div class=""><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo" class=""><b class="">../include/llvm/PassSupport.h:226:8: </b><span style="color:#d53bd3" class=""><b class="">warning: </b></span><b class="">'PassRegistrationListener' has no out-of-line virtual method definitions;</b></div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo" class=""><b class=""> its vtable will be emitted in every translation unit [-Wweak-vtables]</b></div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo" class="">struct PassRegistrationListener {</div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo;color:rgb(52,189,38)" class=""><b class=""> ^</b></div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo" class="">In file included from ../lib/IR/Instructions.cpp:16:</div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo" class="">In file included from ../lib/IR/LLVMContextImpl.h:19:</div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo" class="">In file included from ../lib/IR/ConstantsContext.h:25:</div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo" class=""><b class="">../include/llvm/Support/raw_ostream.h:321:7: </b><span style="color:#d53bd3" class=""><b class="">warning: </b></span><b class="">'raw_pwrite_stream' has no out-of-line virtual method</b></div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo" class=""><b class=""> definitions; its vtable will be emitted in every translation unit [-Wweak-vtables]</b></div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo" class="">class raw_pwrite_stream : public raw_ostream {</div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo;color:rgb(52,189,38)" class=""><b class=""> ^</b></div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo" class=""><b class="">../include/llvm/Support/raw_ostream.h:539:7: </b><span style="color:#d53bd3" class=""><b class="">warning: </b></span><b class="">'buffer_ostream' has no out-of-line virtual method definitions;</b></div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo" class=""><b class=""> its vtable will be emitted in every translation unit [-Wweak-vtables]</b></div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo" class="">class buffer_ostream : public raw_svector_ostream {</div></div><div class=""><br class=""></div><div class="">However, I do wonder how useful my patch for Instructions.h (and constants.h too which needed work), when you can’t #include PassSupport without -fno-rtti.</div><div class=""><br class=""></div><div class="">So, i’l get a patch out soon to look at, but i would like to discuss whether any of you think its worth it given the other warnings which still prevent no-rtti.</div><div class=""><br class=""></div><div class="">Cheers,</div><div class="">Pete</div><div class=""><div class="h5"><div class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Aug 7, 2015, at 10:54 AM, Pete Cooper via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank" class="">llvm-dev@lists.llvm.org</a>> wrote:</div><br class=""><div class=""><div dir="auto" class=""><div class="">Sorry about this. I think if I'd finished the work to remove vtables from Value then it wouldn't be an issue, but I put that on hold due to performance concerns.</div><div class=""><br class=""></div><div class="">I can add back in a bunch of anchor functions where needed. Will just need to go through and find all the classes which need them.</div><div class=""><br class=""></div><div class="">Pete<br class=""><br class="">Sent from my iPhone</div><div class=""><br class="">On Aug 7, 2015, at 10:32 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="">dblaikie@gmail.com</a>> wrote:<br class=""><br class=""></div><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Fri, Aug 7, 2015 at 10:22 AM, Hans Wennborg via llvm-dev <span dir="ltr" class=""><<a href="mailto:llvm-dev@lists.llvm.org" target="_blank" class="">llvm-dev@lists.llvm.org</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Thu, Aug 6, 2015 at 12:04 PM, David Chisnall via llvm-dev<br class="">
<<a href="mailto:llvm-dev@lists.llvm.org" target="_blank" class="">llvm-dev@lists.llvm.org</a>> wrote:<br class="">
> [Ooops, sent to the old list address by mistake]<br class="">
><br class="">
> On 30 Jul 2015, at 21:04, <a href="mailto:tom@stellard.net" target="_blank" class="">tom@stellard.net</a> wrote:<br class="">
>><br class="">
>> For flags like -fno-rtti (are there others?) that are required in some cases<br class="">
>> (I think -fno-rtti is required only if you sub-class LLVM objects), I would propose<br class="">
>> adding a separate flag like --uses-rtti. This would give users more fine-grained<br class="">
>> control over which flags they use, and also would let them choose the correct<br class="">
>> flag since, for example, -fno-rtti is not understood by MSVC.<br class="">
><br class="">
> There appears to be a regression in LLVM 3.7, which means that you must compile with -fno-rtti if you include llvm’s Instructions.h. The issue is that a few of the classes (ICmpInst, GetElementPtrInst and PHINode) are now defined entirely in the header, so every compilation unit that includes the header will emit them. These classes all inherit from Instruction (indirectly via CmpInst in the case of ICmpInst) and so fail to link if compiled with -fno-rtti, because they can’t find the vtable for ICmpInst.<br class="">
<br class="">
</span>I looked at the file, and this didn't seem true (e.g.<br class="">
GetElementPtrInst::init is still out-of-line). But then I realized you<br class="">
mean virtual functions, so these classes no longer have a key<br class="">
function.<br class="">
<br class="">
This is probably Pete's r240588. I suppose we could add key functions<br class="">
to these classes (even if they're not used for anything). I'm not sure<br class="">
how we'd prevent this from regressing though :-/<br class=""></blockquote><div class=""><br class="">In theory the LLVM style guide mandates key functions for all dynamic classes (under the claim of build performance - avoiding duplicate vtable emission, etc). We've never strongly enforced it though - if we really wanted to, we could do so as Clang has a warning that triggers whenever a vtable is emitted weakly (which is what happens when there isn't a key function).<br class=""><br class="">- David<br class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="">
- Hans<br class="">
<div class=""><div class="">_______________________________________________<br class="">
LLVM Developers mailing list<br class="">
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank" class="">llvm-dev@lists.llvm.org</a> <a href="http://llvm.cs.uiuc.edu/" rel="noreferrer" target="_blank" class="">http://llvm.cs.uiuc.edu</a><br class="">
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br class="">
</div></div></blockquote></div><br class=""></div></div>
</div></blockquote></div>_______________________________________________<br class="">LLVM Developers mailing list<br class=""><a href="mailto:llvm-dev@lists.llvm.org" target="_blank" class="">llvm-dev@lists.llvm.org</a> <a href="http://llvm.cs.uiuc.edu/" target="_blank" class="">http://llvm.cs.uiuc.edu</a><br class=""><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" target="_blank" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br class=""></div></blockquote></div><br class=""></div></div></div></div></blockquote></div><br class=""></div></div>
</div></blockquote></div><br class=""></body></html>