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