[PATCH] SectionMemoryManager is not a JITMemoryManager

[Filip Pizlo]

On May 7, 2013, at 2:37 AM, David Tweed <david.tweed at arm.com> wrote:

> |It does update unit tests. :-). MCJITTest Base is changed. 
> | Are there other places that I don't know about?
> 
> Ah, I was wondering if any of the actual code in MCJITTEst.cpp now needed,
> or perhaps more
> likely, would be simplified by changing: it looks
> like the
> 'static_cast<SectionMemoryManager*>(MM)->invalidateInstructionCache()'
> instructions
> can go now that MM is definitely a SectionMemoryManager* whose
> applyPermissions() does that at
> the end of the it. (Since the purpose is to test the built-in "default"
> manager it seems
> worthwhile to use its behaviour rather than write code that might be needed
> if a "user"
> memory manager were being used.)

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?

-Filip

> 
> Cheers,
> Dave
> 
> -Filip
> 
> On May 7, 2013, at 12:58 AM, David Tweed <david.tweed at arm.com> wrote:
> 
>> Hi,
>> 
>> I haven't had time to do more than skim the patch, but one thing is that I
>> notice there aren't any changes to any of the tests. I know that we've
>> unfortunately got a situation where several of the MCJIT using tests just
>> copy and paste whatever memory management code lli was using at the time
> so
>> they easily end up out-of-sync. If after this patch is applied they still
>> need updating I'll have a go at it, although it might be preferable if
>> someone more knowledgeable has a look... (I just about know how to what
>> needs to be done one ARM, but I may well not understand the other
> supported
>> but less mainstream architectures.)
>> 
>> Will try to come up with other comments on the patch today,
>> 
>> Cheers,
>> Dave
>> 
>> -----Original Message-----
>> From: llvm-commits-bounces at cs.uiuc.edu
>> [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Filip Pizlo
>> Sent: 04 May 2013 20:37
>> To: llvm-commits at cs.uiuc.edu
>> Subject: [PATCH] SectionMemoryManager is not a JITMemoryManager
>> 
>> This patch cleans up the relationship between RTDyldMemoryManager,
>> JITMemoryManager, and EngineBuilder.
>> 
>> As I understand it, JITMemoryManager is meant to go away at some point,
> and
>> RTDyldMemoryManager is not meant to be predestined to have to support all
> of
>> the things that JITMemoryManager had; it may have some of the same things,
>> but also maybe some new things, and probably not all of the old things.
>> 
>> But right now EngineBuilder doesn't know anything about
> RTDyldMemoryManager
>> and assumes that JITMemoryManager is the way memory is managed in both
> MCJIT
>> and the old JIT.  This means that if you want to pass an
> RTDyldMemoryManager
>> to the MCJIT via the EngineBuilder API, you have to actually create a
>> JITMemoryManager and stub out a *bunch* of methods.  SectionMemoryManager
>> does this, for example: SectionMemoryManager is a subtype of
>> JITMemoryManager that cannot actually be used as a JITMemoryManager,
> except
>> passing it through EngineBuilder so that MCJIT can use it as an
>> RTDyldMemoryManager. :-/
>> 
>> This patch attempts to fix the problem:
>> 
>> - EngineBuilder now knows that RTDyldMemoryManager and JITMemoryManager
> are
>> different but similar things.  It knows that if you set a
> JITMemoryManager,
>> then it can be used for the MCJIT.  It knows that an RTDyldMemoryManager
> can
>> also be used for the MCJIT, but cannot be used for old JIT.  It will
> ensure
>> that you cannot set both at the same time since that would be confusing
>> (setting one overrides the other).
>> 
>> - SectionMemoryManager is no longer a subtype of JITMemoryManager, and no
>> longer has a ton of stub unimplemented methods.
>> 
>> - lli and various tests are changed to know that a SectionMemoryManager is
>> not a JITMemoryManager.
>> 
>> This is a change to the C++ API, and I actually don't know that the
> policies
>> on this are!  But I hope that this is something that we can change, since
> it
>> kills some gnarly code and concentrates whatever little ugliness remains
>> into EngineBuilder (it's only ugly because it has to know that there are
> two
>> kinds of ways of saying that you want to manager memory yourself).
>> 
>> -Filip

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130509/d1f218dc/attachment.html>