<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<meta name="Generator" content="Microsoft Word 14 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
span.EmailStyle17
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri","sans-serif";}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Hi Yaron,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Overall this looks great.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></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.)<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></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.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></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?<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></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.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></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.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></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!<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">-Andy<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></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:yaron.keren@gmail.com]
<br>
<b>Sent:</b> Monday, October 21, 2013 10:57 PM<br>
<b>To:</b> <llvmdev@cs.uiuc.edu>; Kaylor, Andrew<br>
<b>Subject:</b> SmallPtrSet patch for MCJIT<o:p></o:p></span></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">Hi Andy,<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Here is the patch. it incorporates:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">1) your latest patch to SVN.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">2) mcjit-module-state-optimization.patch.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">3) the PtrSet changes.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></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.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">I got bitten by subtle bugs arising from MCJIT inheriting from EE: <o:p></o:p></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.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">I know it's in the plans, yet more reasons to do so.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></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.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Yaron<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</div>
</body>
</html>