Add EngineBuilder to unify the ExecutionEngine creation API.

[Jeffrey Yasskin, 2009-07-09 04:52:48]

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?

[Reid Kleckner, 2009-07-14 03:29:38]

I went through and changed uses of the old API to the new API.  In some places
it's a win, and others it's not.

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;
On 2009/07/09 04:52:48, Jeffrey Yasskin wrote:
> With the ModuleProvider #include, you don't need this forward declaration. I
> think it's up in the air whether you want the #include.

I put the include so I could make an overloaded EngineBuilder constructor that
takes a Module * instead of a ModuleProvider *.  I could of course move it to
the .cpp file, but the code for it is so short that I put it inline.  I guess
you saw that, though.  I'll move it out of line.

http://codereview.appspot.com/91086/diff/1001/1002#newcode380
Line 380: /// stack-allocating a builder and chaining the various set* methods
and
On 2009/07/09 04:52:48, Jeffrey Yasskin wrote:
> s/A and B and C/A, B, and C/

Done.

http://codereview.appspot.com/91086/diff/1001/1002#newcode387
Line 387: INTERP,
On 2009/07/09 04:52:48, Jeffrey Yasskin wrote:
> I'd spell this out. There's no reason to save keystrokes on a constant that'll
> be typed as rarely as this one.

Done.

http://codereview.appspot.com/91086/diff/1001/1002#newcode393
Line 393: EnginePreference Pref;
On 2009/07/09 04:52:48, Jeffrey Yasskin wrote:
> I'd call this WhichEngine.

Done.

http://codereview.appspot.com/91086/diff/1001/1002#newcode416
Line 416: EngineBuilder(Module *m) : MP(new ExistingModuleProvider(m)) {
On 2009/07/09 04:52:48, Jeffrey Yasskin wrote:
> You could avoid needing the ModuleProvider.h #include by moving this to the
.cc
> file.

Done.

http://codereview.appspot.com/91086/diff/1001/1002#newcode420
Line 420: EngineBuilder(ModuleProvider *mp) : MP(mp) {
On 2009/07/09 04:52:48, Jeffrey Yasskin wrote:
> This function should document whether the created ExecutionEngine takes
> ownership of the ModuleProvider.

Done.

http://codereview.appspot.com/91086/diff/1001/1002#newcode424
Line 424: EngineBuilder &setEnginePreference(EnginePreference p) {
On 2009/07/09 04:52:48, Jeffrey Yasskin wrote:
> Not a great name. Maybe "setEngineToCreate"?

Sure.  I'm kinda ambivalent about either.

http://codereview.appspot.com/91086/diff/1001/1002#newcode429
Line 429: EngineBuilder &setJITMemoryManager(JITMemoryManager *jmm) {
On 2009/07/09 04:52:48, Jeffrey Yasskin wrote:
> Similarly, this should document whether it takes ownership of the jmm.

Done.

http://codereview.appspot.com/91086/diff/1001/1002#newcode444
Line 444: EngineBuilder &setAllocateGVsWithCode(bool a) {
On 2009/07/09 04:52:48, Jeffrey Yasskin wrote:
> You should comment what this function actually changes.

Done.

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);
On 2009/07/09 04:52:48, Jeffrey Yasskin wrote:
> 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.

Done.

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);
On 2009/07/09 04:52:48, Jeffrey Yasskin wrote:
> Why did this get to go away?

It was also present in EngineBuilder::create, which ends up calling this, so I
eliminated the extra copy.  However, there are clients of the API that call
EE::createJIT directly.  I'm not sure if it's needed for the interpreter, but I
guess I'll leave it in both places since it was working before.

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
------------------===//
On 2009/07/09 04:52:48, Jeffrey Yasskin wrote:
> Is the description here still wrong?

I suppose I could drop "code emitter" and it would be more true.

[Jeffrey Yasskin, 2009-07-14 03:43:55]

LGTM. Please send it on to llvm-commits. Thanks!

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

http://codereview.appspot.com/91086/diff/1040/50#newcode422
Line 422: EngineBuilder &setEngineToCreate(EngineToCreate w) {
Each of these methods should document its default. We can see that by looking at
the definition of init(), but people also use the doxygen.

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

http://codereview.appspot.com/91086/diff/1040/66#newcode401
Line 401: EngineBuilder builder(M);
You could put this all on one line.