[PATCH] SectionMemoryManager is not a JITMemoryManager

[Filip Pizlo]

Thanks for the feedback!

I really like the MCJITMemoryManager intermediate class idea. 

I'm ok with pausing on this for a day or so. But I'm also really excited about getting to the point where WebKit can supply a MM to the MCJIT, and if I did that now it would be *ugly*. Hence my desire to simplify.

-Filip

On May 6, 2013, at 4:41 PM, "Kaylor, Andrew" <andrew.kaylor at intel.com> wrote:

> I definitely like the patch and this is the direction I was hoping to see this move.  I only have a couple of concerns.
> 
> First, I don't like the propagation of the name "RTDyldMemoryManager" at the EngineBuilder level.  It's quite a mouthful, and it exposes an implementation detail.  The RuntimeDyld component is designed to be able to function alone outside MCJIT, and that's why it has its own memory manager class.  The fact that MCJIT is implemented in terms of that memory manager class is something that isn't likely to change, but it's still an artifact of the implementation.  I'm not convinced we want people using MCJIT to be thinking about RuntimeDyld.
> 
> I'm not sure I have an alternative that I like better to offer, but I don't want to do this incorrectly.  The layering of components in this area has been tricky to keep straight in the past, and I want to be careful not to take any steps in the wrong direction.  I don't want to introduce a method to setRTDyldMemoryManager when what we're really setting the memory manager for is MCJIT.  There may come a time when MCJIT needs to do some additional things to manage memory that aren't connected to RuntimeDyld.
> 
> I'm inclined to think that it would be best to introduce an MCJITMemoryManager class that did nothing but wrap RTDyldMemoryManager for interface purposes and call the new method EngineBuilder::setMCJITMemoryManager().
> 
> At the same time, I'm not thrilled with having an interface with setJITMemoryManager and setMCJITMemoryManager methods co-existing.  The underlying implementations might be getting cleaner, but the EngineBuilder is getting uglier and more confusing.  When I first read this patch, I thought it wasn't going to work with things like LLDB that use MCJIT but have memory managers implemented using the DefaultJITMemoryManager.  I see on closer inspection that I was wrong, but my first impression is a hint as to the resultant usability of the EngineBuilder interface, which has already been a source of confusion.
> 
> I'd like to see some variant of this patch go in, but I'd like to give it a few days to stew and solicit input from other interested parties in the meantime.
> 
> -Andy
> 
> -----Original Message-----
> From: Rafael Espíndola [mailto:rafael.espindola at gmail.com] 
> Sent: Monday, May 06, 2013 12:50 PM
> To: Filip Pizlo
> Cc: llvm-commits at cs.uiuc.edu; Kaylor, Andrew
> Subject: Re: [PATCH] SectionMemoryManager is not a JITMemoryManager
> 
> I love this patch, but I am new to MCJIT too. Andrew, what do you think?
> 
> On 4 May 2013 15:37, Filip Pizlo <fpizlo at apple.com> wrote:
>> 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
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>