[llvm-commits] Adding a builder for ExecutionEngines

[Jeffrey Yasskin]

I commented through Rietveld, but that doesn't integrate with
llvm-commits. Of course, any comments Evan makes override mine.



http://codereview.appspot.com/91086/diff/1001/1002
File include/llvm/ExecutionEngine/ExecutionEngine.h (right):

http://codereview.appspot.com/91086/diff/1001/1002#newcode37
Line 37: class ModuleProvider;
With the ModuleProvider #include, you don't need this forward
declaration. I think it's up in the air whether you want the #include.

http://codereview.appspot.com/91086/diff/1001/1002#newcode380
Line 380: /// stack-allocating a builder and chaining the various set*
methods and
s/A and B and C/A, B, and C/

http://codereview.appspot.com/91086/diff/1001/1002#newcode387
Line 387: INTERP,
I'd spell this out. There's no reason to save keystrokes on a constant
that'll be typed as rarely as this one.

http://codereview.appspot.com/91086/diff/1001/1002#newcode393
Line 393: EnginePreference Pref;
I'd call this WhichEngine.

http://codereview.appspot.com/91086/diff/1001/1002#newcode416
Line 416: EngineBuilder(Module *m) : MP(new ExistingModuleProvider(m)) {
You could avoid needing the ModuleProvider.h #include by moving this to
the .cc file.

http://codereview.appspot.com/91086/diff/1001/1002#newcode420
Line 420: EngineBuilder(ModuleProvider *mp) : MP(mp) {
This function should document whether the created ExecutionEngine takes
ownership of the ModuleProvider.

http://codereview.appspot.com/91086/diff/1001/1002#newcode424
Line 424: EngineBuilder &setEnginePreference(EnginePreference p) {
Not a great name. Maybe "setEngineToCreate"?

http://codereview.appspot.com/91086/diff/1001/1002#newcode429
Line 429: EngineBuilder &setJITMemoryManager(JITMemoryManager *jmm) {
Similarly, this should document whether it takes ownership of the jmm.

http://codereview.appspot.com/91086/diff/1001/1002#newcode444
Line 444: EngineBuilder &setAllocateGVsWithCode(bool a) {
You should comment what this function actually changes.

http://codereview.appspot.com/91086/diff/1001/1009
File lib/ExecutionEngine/ExecutionEngine.cpp (right):

http://codereview.appspot.com/91086/diff/1001/1009#newcode394
Line 394: EngineBuilder builder(MP);
You should be able to write this as
"EngineBuilder(MP).setEnginePreference(...)....create()". If that didn't
work, I'd like to figure out why and try to fix it.

http://codereview.appspot.com/91086/diff/1001/1006
File lib/ExecutionEngine/JIT/JIT.cpp (left):

http://codereview.appspot.com/91086/diff/1001/1006#oldcode208
Line 208: sys::DynamicLibrary::LoadLibraryPermanently(0, ErrorStr);
Why did this get to go away?

http://codereview.appspot.com/91086/diff/1001/1003
File unittests/ExecutionEngine/JIT/JITTest.cpp (right):

http://codereview.appspot.com/91086/diff/1001/1003#newcode1
Line 1: //===- JITTest.cpp - Unit tests for the JIT code emitter
------------------===//
Is the description here still wrong?

http://codereview.appspot.com/91086


On Wed, Jul 8, 2009 at 4:35 PM, Reid Kleckner<rnk at mit.edu> wrote:
> In the last patch I was working on, Evan asked if I could add a
> builder for ExecutionEngines, since the current API call is kind of
> unwieldy.  It has four optional arguments, and a fifth if you count
> the JIT memory manager.  I attempted to unify this all into just one
> EngineBuilder class, which makes it so you only have to specify the
> arguments you want to control with chained setter methods.
>
> Does this seem like a better API?  Please review.
>
> Patch attached and linked:
> http://codereview.appspot.com/91086/show
>
> Thanks,
> Reid
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>