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

Tue Apr 30 08:50:07 PDT 2013

On Apr 30, 2013, at 4:42 AM, Eric Christopher <echristo at gmail.com> wrote:

> 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?

It would seem strange to sort of draw the line here. Wrap.h already has a bunch if unrelated stuff. Perhaps instead of drawing the line at CodeModel it would be better to split up Wrap?

I would favor the C++ header that declares a type to also have the C wrapper and include the C header. 

> 
> -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
>> 