<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><br></div><div>Answer inline.</div><br><div><div>On Dec 6, 2012, at 2:51 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-family: arial, helvetica, sans-serif; font-size: 10pt"><div dir="ltr">On Thu, Dec 6, 2012 at 2:46 PM, Pedro Artigas <span dir="ltr"><<a href="mailto:partigas@apple.com" target="_blank">partigas@apple.com</a>></span> wrote:<br>
<div class="gmail_extra"><div class="gmail_quote"><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; position: static; z-index: auto; ">Sure, no problem.<br>
<br>
The MCContext is used inside MachineModuleInfo which is a ImmutablePass under the pass manager model, it can also be used standalone. the change added a boolean that indicates if someone will call doInitialization / doFinalization to reuse the MCContext data structure across multiple modules or not, today only MachineModuleInfo does that, and, since it is a Pass it gets it's doInitialization/doFinalization called automagically by the pass manager and calls the MCContext doInitialization / doFinalization methods there.<br>
<br>
The issue was that since there are some uses of the MCContext that are outside of the PassManager, those need to have doInitialization and doFinalization called automatically by constructor/destructor as otherwise nobody would call doInitialization/doFinalization (which were the root cause of valgrind bugs in my prior check in)<br>
<br>
Is that sufficient context?<br></blockquote><div><br></div><div style="">This is a fantastic explanation. In the future, I would love to see this type of explanation in the commit log, it helps when reviewing the patch.</div>
<div style=""><br></div><div style="">As a side-note, I feel it might be a slightly better design to have an MCContext, and an MCContextPass, where the MCContext doesn't have doInitialization / doFinalization (and in fact, doesn't derive from the Pass machinery), but the latter does. The latter can then construct and destroy the MCContext for each module. This design would make the bool flag unnecessary by encoding the two interfaces in the type system itself.</div>
<div style=""><br></div><div style="">Would that make sense? Or is it really important performance wise to just clear the data structures inside MCContext rather than destroying and recreating them?</div></div></div></div></div></blockquote><div><br></div><div>It does make sense to have a MCContextManual and MCContextPass or some other name like this (the non Manual/Pass version, being the more specialized, should derive from the regular/Manual one). </div><div><br></div><div>This would save the bool inside the class, (which is insignificant as it is a single bool in a class that has just a single instanced object) but I believe would make the code a little cleaner. Both approaches achieve pretty much the same thing with regards to compile time savings.</div><div><br></div><div>It is important to just clear the data structures, that is the source of compile time savings as there is no extra allocate/deallocate.</div><div><br></div><br><blockquote type="cite"><div style="font-family: arial, helvetica, sans-serif; font-size: 10pt"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks<br>
<span class="HOEnZb"><font color="#888888"><br>
Pedro<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
On Dec 6, 2012, at 2:32 PM, Jim Grosbach <<a href="mailto:grosbach@apple.com">grosbach@apple.com</a>> wrote:<br>
<br>
> Hi Pedro,<br>
><br>
> I don't think I sufficiently understand the context (pun not intended, but a nice side effect) here to follow. Can you elaborate a bit on what you mean by "the MCContext doInitialization/doFinalization issues" and why this patch addresses them?<br>
><br>
> -Jim<br>
><br>
> On Dec 6, 2012, at 1:52 PM, Pedro Artigas <<a href="mailto:partigas@apple.com">partigas@apple.com</a>> wrote:<br>
><br>
>> Hello All,<br>
>><br>
>> Self explanatory<br>
>><br>
>> Thanks<br>
>><br>
>> Pedro<br>
>><br>
>> <mccontextinitfinafix.patch>_______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div></div>
</blockquote></div><br></body></html>