[PATCH][llvm-c] Expose CodeModel and EnableFastISel through the MCJIT C API

[Eric Christopher]

Few comments on the patch:

+#include "llvm-c/TargetMachine.h"
+#include "llvm/Support/CodeGen.h"'

+  inline CodeModel::Model unwrap(LLVMCodeModel Model) {
+    switch (Model) {

I'd really like to keep these out of Wrap.h. I've been trying to keep
it more minimal and this is adding dependencies everywhere.

Could you localize these a bit closer to the actual uses?

-eric


On Tue, Apr 30, 2013 at 2:13 AM, Filip Pizlo <fpizlo at apple.com> wrote:
> Updated patch, which fixes line lengths.
>
>
>
> -F
>
>
> On Apr 29, 2013, at 5:38 PM, "Kaylor, Andrew" <andrew.kaylor at intel.com>
> wrote:
>
> I don't think I'd say the problems with code models and fast isel are
> blockers.  You can get into the same problems with the C++ interface.
> Mostly I wanted to mention it as a first place to look if things go haywire
> when you try new combinations.  Some combinations don't usually make sense
> (like small code model on x86-64), but others just fail because we don't
> have all the necessary relocations implemented.  There's no reason for the
> C-interface to be any more restrictive than the C++ interface in this
> regard.
>
> If you clean up the line lengths this should be ready to go.
>
> -Andy
>
> -----Original Message-----
> From: Filip Pizlo [mailto:fpizlo at apple.com]
> Sent: Monday, April 29, 2013 5:25 PM
> To: Kaylor, Andrew
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [PATCH][llvm-c] Expose CodeModel and EnableFastISel through the
> MCJIT C API
>
>
>
> On Apr 29, 2013, at 4:25 PM, "Kaylor, Andrew" <andrew.kaylor at intel.com>
> wrote:
>
> Inserting the new CodeModel member of the LLVMMCJITCompilerOptions into the
> middle of the previous definition would cause problem for anyone using the
> old structure.  Obviously there hasn't been time for that to be a big deal,
> but is there a reason you did that?
>
>
> True!  I did because of compactness; I believe that the enum is 4 bytes and
> the bio leans are bytes. I wanted the strict to be well packed.
>
>
> The way you changed initializing options seems pretty reasonable.
>
> Several lines in your patch go over 80 characters.  At least one line in
> your previous patch did that too, but I missed it.  LLVM coding standards
> are against that.
>
>
> Oh, I'm sorry!  I will make sure I check that before submitting.
>
>
> You should be aware that not all code models work well with MCJIT.  We've
> also had some problems with the fast instruction selectors.  Neither of
> these things is intentional so much as a current pitfall to be aware of.
>
>
> Thanks for the heads up. Do you think this is a blocker for exposing these
> options, or just an FYI?
>
>
> -Andy
>
> -----Original Message-----
> From: Filip Pizlo [mailto:fpizlo at apple.com]
> Sent: Monday, April 29, 2013 3:35 PM
> To: llvm-commits at cs.uiuc.edu
> Cc: Kaylor, Andrew
> Subject: [PATCH][llvm-c] Expose CodeModel and EnableFastISel through
> the MCJIT C API
>
> There are more options that are available through the C++ API but not the C
> API; this patch is an incremental attempt to expose more of those options.
> It exposes:
>
> CodeModel: It's now possible to create an MCJIT instance with any CodeModel
> you like.  Previously it was only possible to create an MCJIT that used
> CodeModel::JITDefault.
>
> EnableFastISel: It's now possible to turn on the fast instruction selector.
>
> The CodeModel option required some trickery, and I hope that the balance I
> struck is sensible.  The problem is that previously, we were ensuring future
> binary compatibility in the MCJITCompilerOptions by mandating that the user
> bzero's the options struct and passes the sizeof() that he saw; the bindings
> then bzero the remaining bits.  This works great but assumes that the
> bitwise zero equivalent of any field is a sensible default value.
>
> But this is not the case for LLVMCodeModel, or its internal equivalent,
> llvm::CodeModel::Model.  In both of those, the default for a JIT is
> CodeModel::JITDefault (or LLVMCodeModelJITDefault), which is not bitwise
> zero.
>
> So I changed this aspect of the API.  Instead of requiring the user to bzero
> the options struct, I'm now requiring that the user calls
> LLVMInitializeMCJITCompilerOptions() to initialize the options to the
> defaults.  The idiom, as shown in the C API Test, looks like:
>
> LLVMMCJITCompilerOptions options;
> LLVMInitializeMCJITCompilerOptions(&options, sizeof(options));
> options.OptLevel = 2;  options.NoFramePointerElim = false; // Just
> ensure that this field still exists.
>
> LLVMExecutionEngineRef engine;
> ASSERT_EQ(
>   0, LLVMCreateMCJITCompilerForModule(&engine, module, &options,
> sizeof(options),
>                                       &error));
>
> This still ensures binary future binary compatibility, but lifts the
> restriction that bitwise zero must be a sensible default. Binary and source
> compatibility is ensured with this idiom because:
>
> New LLVM version at build time: if client code was written against an old
> version of LLVM, but was then built against a newer version, then the user's
> code would evaluate sizeof(options) differently.  Though the user was
> unaware of the new fields, LLVMInitializeMCJITCompilerOptions() would know
> to initialize those fields.
>
> New LLVM version at run time: if client code was built against an old
> version of LLVM, then it would have an old value of sizeof(options).
> LLVMInitializeMCJITCompilerOptions() would only initialize those fields that
> were within the client's value of sizeof(options).  Then,
> LLVMCreateMCJITCompilerForModule() would fill in the remaining fields.
>
> This patch also provides common helpers for wrapping and unwrapping
> CodeModel.  Previously this was done directly in TargetMachineC.cpp, but now
> we also need it in ExecutionEngineBindings.cpp.  Note that wrap(CodeModel)
> is currently unused; I don't know what the policy on dead code is; I went
> with the assumption that erring on the side of completeness (i.e. having
> symmetry in available wrap/unwrap functions) is the more sensible choice in
> this particular case.
>
> Finally, this patch *does* break binary compatibility with earlier LLVM
> revisions that used a different API for creating the MCJIT.  I'm assuming
> that this is fine since none of this has been officially released yet.  I'm
> hopeful that the protocol for options is now robust enough that we won't
> have to mess with it again.
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>