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

[Kaylor, Andrew]

So should I commit Filip's patch with the understanding that the wrap stuff will be split out later, or should I wait for another revision?

-Andy

-----Original Message-----
From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Eric Christopher
Sent: Tuesday, April 30, 2013 4:34 AM
To: Filip Pizlo
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [PATCH][llvm-c] Expose CodeModel and EnableFastISel through the MCJIT C API

Yeah, I admit to being awful at naming things. Sounds good. Please commit separately or I can do it if you'd like.

-eric

On Tue, Apr 30, 2013 at 4:53 AM, Filip Pizlo <fpizlo at apple.com> wrote:
> Eric introduced Wrap.h some revisions ago, and I actually don't mind 
> that name. But what CBindingWrapping.h?
>
> -Filip
>
> On Apr 29, 2013, at 8:38 PM, Evan Cheng <evan.cheng at apple.com> wrote:
>
> Hi Filip,
>
> Sorry I missed your previously patch. I just have a nitpick about 
> Wrap.h. It seems like an awfully generic name. Is it possible to come 
> up with a more descriptive one?
>
> Evan
>
> On Apr 29, 2013, at 6:13 PM, Filip Pizlo <fpizlo at apple.com> wrote:
>
> Updated patch, which fixes line lengths.
>
> <mcjit.patch>
>
> -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
>
>
>
> _______________________________________________
> 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