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

[Filip Pizlo]

Sounds great, I will fix line lengths.

For future reference, are there some handy tools to check line lengths quickly?  I'm an Emacs user and Emacs makes it all too easy for me to end up with a long line that looks just fine for me, but that is actually horribly long.

-Filip


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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130429/d593f424/attachment.html>