[llvm-commits] [PATCH] unit tests for MCJIT

[Malea, Daniel]

On 2012-09-25, at 11:57 AM, Jim Grosbach wrote:

> 
> On Sep 25, 2012, at 8:22 AM, "Malea, Daniel" <daniel.malea at intel.com> wrote:
> 
>> On 2012-09-24, at 5:51 PM, Jim Grosbach wrote:
>> 
>> 
>> On Sep 24, 2012, at 2:20 PM, "Du Toit, Stefanus" <stefanus.du.toit at intel.com<mailto:stefanus.du.toit at intel.com>> wrote:
>> 
>> -----Original Message-----
>> From: llvm-commits-bounces at cs.uiuc.edu<mailto:llvm-commits-bounces at cs.uiuc.edu> [mailto:llvm-commits-
>> bounces at cs.uiuc.edu] On Behalf Of Jim Grosbach
>> Sent: Monday, September 24, 2012 1:38 PM
>> To: Malea, Daniel
>> Cc: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
>> Subject: Re: [llvm-commits] [PATCH] unit tests for MCJIT
>> 
>> The most substantive comment is that we shouldn't move the memory
>> manager out of lli.cpp. That's very much not a general purpose memory
>> manager, but rather is intended as a (trivial) example of what a client
>> application may need to implement in order to utilize the MCJIT. It's not
>> part of a general LLVM library, but rather part of the lli tool.
>> 
>> What's the downside of moving the memory manager out of lli? If it's sufficient for a simple client (like lli's) needs, why not have it be reusable?
>> 
>> My understanding is that the tests do need *some* memory manager implementation. If it's not moved here, wouldn't it need to be included from inside of the lli sources (awkward) or duplicated somewhere in the test sources (even more awkward)?
>> 
>> It's a division of responsibility and layering thing. The MCJIT library defines the interface to the memory manager, but that's it. The client is responsible for supplying an implementation. We want the implementation to make this distinction very starkly at least for now, and moving the memory manager into the library rather than part of lli obfuscates that purpose. It's possible we'll create another library later that supplies, for example, a simple default hosted JIT environment complete with simple memory manager and refactor lli to reference that. That's an interesting thing and I'm open to it as a design direction, but it's orthogonal to getting unit tests for other parts of the MCJIT and I'd prefer it be dealt with separately.
>> 
>> -Jim
>> 
>> 
>> Thanks for the review Jim! I will address the review comments and post an updated patch once I get through my post-vacation email backlog...
>> 
>> Are you ok with the approach of using the lli memory manager in the unit tests after factoring it out to a separate (but non-public) header. Although this would make the tests depend on a detail of lli, I would prefer that approach over the alternative of duplicating the memory manager in the tests.
> 
> Where would such a header go?
> 

I was thinking we could put it in the tools/lli directory, and then do something like:

#include "../../../tools/lli/LLIMemoryManager.h"

in the unit tests that require an MCJIT-compatible memory manager. It is, as Stefanus mentioned, pretty awkward, but the upside is that it allows us to have unit tests for the lli memory manager as it potentially grows in the future. However it sounds like this isn't the direction we'd like to go in.

Just to confirm, you'd prefer to have a separate memory manager with the unit tests, rather than the above?


Cheers,
Dan