<div dir="rtl"><div dir="ltr">Hi Andy,</div><div dir="ltr"><br></div><div dir="ltr">I added the runStaticConstructorsDestructors and FindFunctionNamed functions. This also required making them virtual in EE.h.</div><div dir="ltr">

<br></div><div dir="ltr">I'm not sure about FindFunctionNamed:</div><div dir="ltr">In addition to searching finalized modules, should it search Added and Loaded modules?</div><div dir="ltr">If it finds a Function in these, should it compile and finalize it before returning the Function* ?</div>

<div dir="ltr"><br></div><div dir="ltr">I modified the implementation assert to test for ownsModule.</div><div dir="ltr">However, the hasModuleBeenAdded functionality is required to avoid duplicate search in getPointerToFunction.  As you note the name is wrong, I renamed the function to hasModuleBeenAddedButNotLoaded and cleaned the logic at getPointerToFunction() to minimize searches for all cases.</div>

<div dir="ltr"><br></div><div dir="ltr">pop_back() vs. clear() - since I knew there is one module only (we override the AddModule so EE can't get another) that what "popped" into my mind. clear() is just as good, changed.</div>

<div dir="ltr"><br></div><div dir="ltr">EE, JIT, MCJIT inheritance - the interpreter and the JIT would also benefit from a SmallPtrSet module manager rather than the current vector so if it's possible to create a module manager that makes sense for everyone, it could be moved back into EE along with the several duplicated functions.</div>

<div dir="ltr"><br></div><div dir="ltr">I clarified this in ~MCJIT comments and ExecutionEngine.h comment.</div><div dir="ltr"><br></div><div dir="ltr">As English is my second language, you're welcome to correct any errors and clarify.</div>

<div dir="ltr"><br></div><div dir="ltr">Patch attached.</div><div dir="ltr"><br></div><div dir="ltr">Yaron</div><div style dir="ltr"><br></div><div class="gmail_extra" dir="ltr"><br><br><div class="gmail_quote"><div>2013/10/22 Kaylor, Andrew <span dir="ltr"><<a href="mailto:andrew.kaylor@intel.com" target="_blank">andrew.kaylor@intel.com</a>></span></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 lang="EN-US" link="blue" vlink="purple">
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Hi Yaron,<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Overall this looks great.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">There are a couple of remaining holes.  Specifically, MCJIT inherits implementations of the FindFunctionNamed and runStaticConstructorsDestructors methods from
 ExecutionEngine which use the old Modules vector, so you’ll need to override these in MCJIT.  (The implementations are fairly trivial.)<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Beyond that I just have a couple of questions.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">First, OwningModuleContainer::hasModuleBeenAdded seems wrong.  It seems to me that it should be equivalent to OwningModuleContain::ownsModule.  That is, everything
 that has been loaded or finalized has also been added.  Your implementation checks “has been added but not loaded” but it doesn’t seem like that’s necessary anywhere.  Can you eliminate this function and just use OwningModuleContain::ownsModule?<u></u><u></u></span></p>


<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Second, why did you decide to use “Modules.pop_back()” in the MCJIT destructor rather than “Modules.clear()”?  I expect there will only be one module that makes
 it into that list so they should be functionally equivalent, but calling “clear” seems to better indicate the intention.  Also, in the same place, I think “don’t inherit from ExecutionEngine” is a potentially misleading comment, since that’s the class that
 clients actually use to interface with MCJIT.  It would probably be better to say something about disentangling the JIT and MCJIT interfaces and specifically mention that we don’t want the base class to manage our modules.  Keep in mind, however, that in addition
 to JIT and MCJIT, there’s also the Interpreter class which inherits from ExecutionEngine.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">There are a few very basic multiple module tests (multi-module-a.ll, multi-module-sm-pic-a.ll , multi-module-eh-a.ll and cross-module-a.ll in test/ExecutionEngine/MCJIT,
 as well as remote versions) that get run as part of the basic acceptance test suite.  These don’t test the case of running static constructors/destructors in secondary modules, but it shouldn’t be hard to add that.<u></u><u></u></span></p>


<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Definitely feel free to add more test cases!<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">-Andy<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Yaron Keren [mailto:<a href="mailto:yaron.keren@gmail.com" target="_blank">yaron.keren@gmail.com</a>]
<br>
<b>Sent:</b> Monday, October 21, 2013 10:57 PM<br>
<b>To:</b> <<a href="mailto:llvmdev@cs.uiuc.edu" target="_blank">llvmdev@cs.uiuc.edu</a>>; Kaylor, Andrew<br>
<b>Subject:</b> SmallPtrSet patch for MCJIT<u></u><u></u></span></p><div><div class="h5">
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<div>
<p class="MsoNormal">Hi Andy,<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Here is the patch. it incorporates:<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">1) your latest patch to SVN.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">2) mcjit-module-state-optimization.patch.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">3) the PtrSet changes.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Other than the OwnedModules implementation there were other differences between 1) and 2), especially in the Finalize* functions, so please review that I got the right code.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">I got bitten by subtle bugs arising from MCJIT inheriting from EE: <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">First, MCJIT overridden addModule but not removeModule so the EE version was called. Second, both MCJIT and EE constructors take ownership of the module and their destructors try to delete it. Fixed this with a hack in MCJIT destructor
 and a FIXME comment, the real fix is simply getting EE out of the picture. It just interferes.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">I know it's in the plans, yet more reasons to do so.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Finally, I don't know if there are good multi-module test cases. I have run only my one-module use-case (currently - I'm having other-than-MCJIT issues with multiple modules) with the code and after fixing the above bugs the code, debugged
 line by line, appear to work OK.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Yaron<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div>
</div></div></div>
</div>

</blockquote></div><br></div></div>