[PATCH][llvm-c] Expose MC JIT

[Eric Christopher]

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
>