<html><head><meta http-equiv="content-type" content="text/html; charset=utf-8"></head><body dir="auto"><div><br></div><div><br>On May 13, 2013, at 9:00 AM, David Tweed <<a href="mailto:david.tweed@arm.com">david.tweed@arm.com</a>> wrote:<br><br></div><blockquote type="cite"><div><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"><meta name="Generator" content="Microsoft Word 12 (filtered medium)"><style><!--
/* Font Definitions */
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
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-size:10.0pt;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 72.0pt 72.0pt 72.0pt;}
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]--><div class="WordSection1"><p class="MsoNormal"><span style="color:#1F497D">Hi,<o:p></o:p></span></p><p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p><p class="MsoNormal"><span style="color:#1F497D">Thanks for the review.<o:p></o:p></span></p><p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p><p class="MsoNormal"><span style="color:#1F497D">| </span>Wouldn't it be better if the tests did the "right thing" and called TheJIT->finalizeObject()?<o:p></o:p></p><div><p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I think you're probably right. It would also be good if the finalizeObject() declaration had a comment mentioning that's what it's intended use is. AFAICS the only comment is the oblique reference in SectionMemoryManager.h.<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></div><div><p class="MsoNormal"><span style="color:#1F497D">|</span>I'm not sure how much I like SectionMemoryManager::applyPermissions() being specified to also mean flushing cache, but I don't have a strong opinion on that.<o:p></o:p></p></div><div><p class="MsoNormal"><span style="color:#1F497D"><o:p> </o:p></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I think that it may be that applyPermissions might not be quite the right name, but my basic reasoning is that it does do the cache invalidation inside the body, so there should be commentary documenting this fact (particularly since it's allowed for alternative implementations of the memory manager to be used). As you can probably tell, in addition to the professional interest in having MCJIT work well on ARM, I'm interested in having LLVM's JITing facilities be documented enough to be usable by "casual users" without them needing to become JIT implementation experts.</span></p></div></div></div></blockquote><div><br></div><div>I'm totally with you on this! And I agree with that being documented. I just think that calling finalizeObject() is the more canonical way of doing this, even if applyPermissions() also works - and the tests should do the canonical thing unless there is good reason not to.</div><div><br></div><div>I do like the idea of adding more regression tests that cover even the non-canonical ways of doing things just so we can have a way of finding out if they break. But the generic tests should still do the canonical thing since they are not meant to cover the more exotic behaviors. </div><br><blockquote type="cite"><div><div class="WordSection1"><div><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"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">I'll wait a bit for other comments, then respin a patch.</span></p></div></div></div></blockquote><div><br></div><div>Cool!</div><br><blockquote type="cite"><div><div class="WordSection1"><div><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"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Cheers,<o:p></o:p></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Dave<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></div><div><p class="MsoNormal">-Filip<o:p></o:p></p></div></div></div></blockquote></body></html>