<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On May 7, 2013, at 2:37 AM, David Tweed <<a href="mailto:david.tweed@arm.com">david.tweed@arm.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">|It does update unit tests. :-). MCJITTest Base is changed.<span class="Apple-converted-space"> </span><br>| Are there other places that I don't know about?<br><br>Ah, I was wondering if any of the actual code in MCJITTEst.cpp now needed,<br>or perhaps more<br>likely, would be simplified by changing: it looks<br>like the<br>'static_cast<SectionMemoryManager*>(MM)->invalidateInstructionCache()'<br>instructions<br>can go now that MM is definitely a SectionMemoryManager* whose<br>applyPermissions() does that at<br>the end of the it. (Since the purpose is to test the built-in "default"<br>manager it seems<br>worthwhile to use its behaviour rather than write code that might be needed<br>if a "user"<br>memory manager were being used.)<br></div></blockquote><div><br></div><div>I think I'll leave this for a subsequent patch - my current refactoring has no behavioral change; changing the tests to call applyPermissions() would be a behavioral change, at least in the tests themselves, since applyPermissions() does other things as well. It's a change I agree with though, so I'll do it next. :-) Sound good?</div><div><br></div><div>-Filip</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br>Cheers,<br>Dave<br><br>-Filip<br><br>On May 7, 2013, at 12:58 AM, David Tweed <<a href="mailto:david.tweed@arm.com">david.tweed@arm.com</a>> wrote:<br><br><blockquote type="cite">Hi,<br><br>I haven't had time to do more than skim the patch, but one thing is that I<br>notice there aren't any changes to any of the tests. I know that we've<br>unfortunately got a situation where several of the MCJIT using tests just<br>copy and paste whatever memory management code lli was using at the time<br></blockquote>so<br><blockquote type="cite">they easily end up out-of-sync. If after this patch is applied they still<br>need updating I'll have a go at it, although it might be preferable if<br>someone more knowledgeable has a look... (I just about know how to what<br>needs to be done one ARM, but I may well not understand the other<br></blockquote>supported<br><blockquote type="cite">but less mainstream architectures.)<br><br>Will try to come up with other comments on the patch today,<br><br>Cheers,<br>Dave<br><br>-----Original Message-----<br>From:<span class="Apple-converted-space"> </span><a href="mailto:llvm-commits-bounces@cs.uiuc.edu">llvm-commits-bounces@cs.uiuc.edu</a><br>[<a href="mailto:llvm-commits-bounces@cs.uiuc.edu">mailto:llvm-commits-bounces@cs.uiuc.edu</a>] On Behalf Of Filip Pizlo<br>Sent: 04 May 2013 20:37<br>To:<span class="Apple-converted-space"> </span><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>Subject: [PATCH] SectionMemoryManager is not a JITMemoryManager<br><br>This patch cleans up the relationship between RTDyldMemoryManager,<br>JITMemoryManager, and EngineBuilder.<br><br>As I understand it, JITMemoryManager is meant to go away at some point,<br></blockquote>and<br><blockquote type="cite">RTDyldMemoryManager is not meant to be predestined to have to support all<br></blockquote>of<br><blockquote type="cite">the things that JITMemoryManager had; it may have some of the same things,<br>but also maybe some new things, and probably not all of the old things.<br><br>But right now EngineBuilder doesn't know anything about<br></blockquote>RTDyldMemoryManager<br><blockquote type="cite">and assumes that JITMemoryManager is the way memory is managed in both<br></blockquote>MCJIT<br><blockquote type="cite">and the old JIT. This means that if you want to pass an<br></blockquote>RTDyldMemoryManager<br><blockquote type="cite">to the MCJIT via the EngineBuilder API, you have to actually create a<br>JITMemoryManager and stub out a *bunch* of methods. SectionMemoryManager<br>does this, for example: SectionMemoryManager is a subtype of<br>JITMemoryManager that cannot actually be used as a JITMemoryManager,<br></blockquote>except<br><blockquote type="cite">passing it through EngineBuilder so that MCJIT can use it as an<br>RTDyldMemoryManager. :-/<br><br>This patch attempts to fix the problem:<br><br>- EngineBuilder now knows that RTDyldMemoryManager and JITMemoryManager<br></blockquote>are<br><blockquote type="cite">different but similar things. It knows that if you set a<br></blockquote>JITMemoryManager,<br><blockquote type="cite">then it can be used for the MCJIT. It knows that an RTDyldMemoryManager<br></blockquote>can<br><blockquote type="cite">also be used for the MCJIT, but cannot be used for old JIT. It will<br></blockquote>ensure<br><blockquote type="cite">that you cannot set both at the same time since that would be confusing<br>(setting one overrides the other).<br><br>- SectionMemoryManager is no longer a subtype of JITMemoryManager, and no<br>longer has a ton of stub unimplemented methods.<br><br>- lli and various tests are changed to know that a SectionMemoryManager is<br>not a JITMemoryManager.<br><br>This is a change to the C++ API, and I actually don't know that the<br></blockquote>policies<br><blockquote type="cite">on this are! But I hope that this is something that we can change, since<br></blockquote>it<br><blockquote type="cite">kills some gnarly code and concentrates whatever little ugliness remains<br>into EngineBuilder (it's only ugly because it has to know that there are<br></blockquote>two<br><blockquote type="cite">kinds of ways of saying that you want to manager memory yourself).<br><br>-Filip</blockquote></div></blockquote></div><br></body></html>