[PATCH][llvm-c] Expose MC JIT

[David Tweed]

Hi,

Not a comment on the general idea (which seems like a good one), but asking
detail question: some architectures (such as ARM) invalidating cache entries
at more times than, eg, x86. (IIRC ARM needs to invalidate the cache at the
transition time between being "data" and "instructions".) It looks to me
like this can be done in LLVMFinalizeAllObjects(), but I'm cc:ing someone
much, much more knowledgeable about this than me... It's certainly desirable
that the API provides enough points at which the MC JIT is in command that
it can call all the (possibly platform specific) permissions/cache
invalidation operations needed on that memory without user code needing to
do it. 

Other than that, the patch looks like a good patch to me (but again Andrew
is the main authority).

Cheers,
Dave


-----Original Message-----
From: llvm-commits-bounces at cs.uiuc.edu
[mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Eric Christopher
Sent: 22 April 2013 14:22
To: Filip Pizlo
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [PATCH][llvm-c] Expose MC JIT

The API and rationale seem totally reasonable to me. Up to Andy to
approve though.

-eric

On Sun, Apr 21, 2013 at 7:14 PM, Filip Pizlo <fpizlo at apple.com> wrote:
> OK - I have a new patch for review, which incorporates Andrew's feedback.
>
> The patch exposes the MCJIT via the C API.
>
> The current API uses the SectionMemoryManager by default.  I plan to
expose
> the ability to have the C API call back into the client's code, and use
the
> client's own memory manager, in the future.  But even then, the default
will
> be SectionMemoryManager.  Because this requires calling
applyPermissions(),
> I also expose the ExecutionEngine::finalizeObject() method.  This was
tricky
> - I take it that in the future, this method will take a Module*M parameter
> to specify which module to finalize.  In order to not create future
> confusion in the C API, I expose this as LLVMFinalizeAllObjects() and
> specify the API's semantics as being that all objects associated with the
> execution engine should be finalized.
>
> The patch also exposes the NoFramePointerElim option.  The manner in which
> options are exposed is designed for forward compatibility; you supply an
> options struct along with a size which you zero-fill prior to
manipulating.
> This is similar to the idiom I've seen used in other C APIs like
BerkeleyDB.
> I considered having separate C function calls for each option, in the
style
> of the ExecutionEngineBuilder API - but while that idiom feels right to me
> in C++, it feels less C-like.  As well, the current options approach
exposes
> not just parts of the Builder but also part of TargetOptions (namely,
> NoFramePointerElim).  It's also more concise in practice.
>
> I plan to expose more innards through the LLVMMCJITCompilerOptions in the
> future.  I'd be happy to do more of that in one go if that was preferred;
> but I thought that a baby step would be the best thing for now.
>
>
>
> -Filip
>
>
> On Apr 21, 2013, at 6:26 PM, Filip Pizlo <fpizlo at apple.com> wrote:
>
>
> On Apr 15, 2013, at 10:42 AM, "Kaylor, Andrew" <andrew.kaylor at intel.com>
> wrote:
>
> OK, let me start by saying that MCJIT does take ownership of the memory
> manager.  It doesn't use an OwningPtr, which would make this clear, but it
> does delete the pointer in its destructor.  I think this is happening
> because we needed some finer control over when the MM got deleted.  I
should
> probably revisit this and at least add some comments to make it clear
what's
> happening and why.  It might not even be an issue anymore, because I did
> some work a while ago to try to clean up object ownership issues.
>
>
> Actually, I was just confused.  MCJIT deletes MemMgr, which is always
> aliased to Dyld, as far as I can tell.  So I was just wrong. :-)
>
>
> That said, I have been meaning for some time to break apart the JIT and
> MCJIT interfaces.  The fact that they are both abstracted by
ExecutionEngine
> and EngineBuilder complicates that, but it really needs to be done (as you
> are seeing).
>
> For now, would it be possible to have the C-interface provide a wrapper
that
> supplies empty implementations of the irrelevant functions when creating a
> memory manager for MCJIT?
>
>
> I think this is sensible.  I will proceed in this way.
>
> Thanks!
>
>
> -Andy
>
> From: Filip Pizlo [mailto:fpizlo at apple.com]
> Sent: Saturday, April 13, 2013 1:18 AM
> To: Kaylor, Andrew
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [PATCH][llvm-c] Expose MC JIT
>
> Ah - good thing you pointed this out.  I just realized that my patch is
> wrong.  Perhaps I can get some feedback on the best way to architect this.
>
> Here's the problem:
>
> - MCJIT does not take ownership of the memory manager.  Hence allocating
one
> in the constructor is wrong; it'll leak when MCJIT dies.  But deleting the
> memory manager passed to MCJIT would be a change in behavior, and I'm not
> sure if it's in line with either what existing users expect or what was
> intended.  Insofar as the JIT instance corresponds to ownership of
modules,
> it feels like it shouldn't also take ownership of the memory manager; for
> example you might imagine wanting to throw away the MCJIT but keep the
code
> it generated and continue to use the memory manager to track it - and
> eventually free it.  But EngineBuilder currently claims that the
> ExecutionEngine takes ownership of the JMM - I'm assuming that this is
just
> wrong documentation, and that EngineBuilder's use of the same JMM option
for
> both JIT and MCJIT is just not right.
>
> - I'd like to expose SectionMemoryManager and, eventually in a separate
> patch, the ability to create custom RTDyldMemoryManagers via the C API.
I'd
> prefer this to be an RTDyldMemoryManager and not a JITMemoryManager, since
> the latter has a load of methods that are not relevant to MCJIT.  But
> EngineBuilder wants a JITMemoryManager.  This would mean that the C API
> would have to pass its RTDyldMemoryManager via a cast to JITMemoryManager
> just so MCJIT could then use it as an RTDyldMemoryManager again.  Seems
> wrong.  I'm assuming that the correct long-term thing is to fix the
> EngineBuilder to not pass the JMM to the MCJIT, since it's good to expose
> the fact that the MCJIT actually just wants an RTDyldMemoryManager
instead.
>
> In short, I'd like to have a separate EngineBuilder setting for the
> RTDyldMemoryManager.  If this is specified and you end up using the JIT
and
> not MCJIT, you get an error.  If you use the MCJIT, then the
> RTDyldMemoryManager option overrides the JMM option.  Or something
similar.
>
> Does that make sense?
>
> -Filip
>
>
> On Apr 12, 2013, at 5:38 PM, Filip Pizlo <fpizlo at apple.com> wrote:
>
>
> Thanks for the feedback!  I will try this change and see what happens.
>
> -Filip
>
>
> On Apr 12, 2013, at 5:35 PM, "Kaylor, Andrew" <andrew.kaylor at intel.com>
> wrote:
>
> Hi Filip,
>
> I'll take a closer look at your patches on Monday, but my initial input is
> that the default memory manager used should be SectionMemoryManager rather
> than the DefaultJITMemoryManager.
>
> Thanks,
> Andy
>
> From: llvm-commits-bounces at cs.uiuc.edu
> [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Filip Pizlo
> Sent: Friday, April 12, 2013 4:49 PM
> To: llvm-commits at cs.uiuc.edu
> Subject: Re: [PATCH][llvm-c] Expose MC JIT
>
> Revised patches included.
>
> I added additional ruggedizing to the LLVMCreateMCJITCompilerForModule
> function, so that if it detects that the passed struct is larger than
> expected, it reports an error instead of continuing.
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>

_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits