New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support MCJIT #5208

Merged
merged 12 commits into from Jan 17, 2014

Conversation

Projects
None yet
@Keno
Member

Keno commented Dec 21, 2013

This adds MCJIT support (only enabled on 3.4+ though due missing functionality and patches in 3.3). MCJIT is ifdef'ed out for 3.3, so we can merge this soon, so we don't have to keep maintaining a branch that touches so many different locations. For 3.4 with MCJIT, I'm still seeing random failures when running the tests, though sequentially everything is fine. This might well be an MCJIT bug, we'll see.

I ended up going with the shadow module approach during imaging (i.e. everything gets emitted into a giant module and we pull out the functions we're actually running).

Closes #3922
Closes #4418

@timholy

This comment has been minimized.

Show comment
Hide comment
@timholy

timholy Dec 21, 2013

Member

Whoa! Amazing stuff!

Member

timholy commented Dec 21, 2013

Whoa! Amazing stuff!

@tknopp

This comment has been minimized.

Show comment
Hide comment
@tknopp

tknopp Dec 21, 2013

Contributor

Has this a large impact on the performance of compilation?

Contributor

tknopp commented Dec 21, 2013

Has this a large impact on the performance of compilation?

@johnmyleswhite

This comment has been minimized.

Show comment
Hide comment
@johnmyleswhite
Member

johnmyleswhite commented Dec 21, 2013

Nice!

@ArchRobison

This comment has been minimized.

Show comment
Hide comment
@ArchRobison

ArchRobison Dec 21, 2013

Contributor

Looking forward to this, particularly if it fixes the AVX issue #4418.

Contributor

ArchRobison commented Dec 21, 2013

Looking forward to this, particularly if it fixes the AVX issue #4418.

@@ -182,7 +182,6 @@ LLVM_TAR=llvm-$(LLVM_VER).tar.gz
else
LLVM_TAR=llvm-$(LLVM_VER).src.tar.gz
endif
endif

This comment has been minimized.

@vtjnash

vtjnash Jan 3, 2014

Member

this change doesn't seem necessary / related

@vtjnash

vtjnash Jan 3, 2014

Member

this change doesn't seem necessary / related

@@ -142,20 +147,39 @@ static Value *runtime_sym_lookup(PointerType *funcptype, char *f_lib, char *f_na
initnul, f_lib);
libMapGV[f_lib] = libptrgv;
libsym = get_library(f_lib);
assert(libsym != NULL);

This comment has been minimized.

@vtjnash

vtjnash Jan 3, 2014

Member

it's possible that libsym is NULL here (if the library wasn't found). the existing code shouldn't have had a problem with it

@vtjnash

vtjnash Jan 3, 2014

Member

it's possible that libsym is NULL here (if the library wasn't found). the existing code shouldn't have had a problem with it

assert(libsym != NULL);
#ifdef USE_MCJIT
llvm_to_jl_value[libptrgv] = libsym;
#else
*((uv_lib_t**)jl_ExecutionEngine->getPointerToGlobal(libptrgv)) = libsym;

This comment has been minimized.

@vtjnash

vtjnash Jan 3, 2014

Member

if you are using a shadow module, can't you just leave everything uninitialized? (here, at worst, it would incur a second runtime lookup)

@vtjnash

vtjnash Jan 3, 2014

Member

if you are using a shadow module, can't you just leave everything uninitialized? (here, at worst, it would incur a second runtime lookup)

This comment has been minimized.

@vtjnash

vtjnash Jan 3, 2014

Member

nevermind

@vtjnash

vtjnash Jan 3, 2014

Member

nevermind

// MCJIT forces this to have external linkage eventually, so we would clobber
// the symbol of the actual function.
std::string name = f_name;
name = "ccall_" + name;

This comment has been minimized.

@vtjnash

vtjnash Jan 3, 2014

Member

if it's getting external linkage, it should probably have a name like jl_ccall_ or julia_ccall_. I though I heard somewhere that linkers used to be faster if the names were unique in the first few characters -- so maybe it should be jl_<f_name>_ccall?

@vtjnash

vtjnash Jan 3, 2014

Member

if it's getting external linkage, it should probably have a name like jl_ccall_ or julia_ccall_. I though I heard somewhere that linkers used to be faster if the names were unique in the first few characters -- so maybe it should be jl_<f_name>_ccall?

Show outdated Hide outdated src/cgutils.cpp
@@ -16,7 +66,7 @@ static GlobalVariable *stringConst(const std::string &txt)
gv = new GlobalVariable(*jl_Module,
ArrayType::get(T_int8, txt.length()+1),
true,
GlobalVariable::PrivateLinkage,
imaging_mode ? GlobalVariable::PrivateLinkage : GlobalVariable::ExternalLinkage,

This comment has been minimized.

@vtjnash

vtjnash Jan 3, 2014

Member

since LLVM already has an implementation of global static strings, is it more worthwhile replacing this whole method with a call to builder.CreateGlobalStringPtr(txt)?

@vtjnash

vtjnash Jan 3, 2014

Member

since LLVM already has an implementation of global static strings, is it more worthwhile replacing this whole method with a call to builder.CreateGlobalStringPtr(txt)?

Show outdated Hide outdated src/cgutils.cpp
Show outdated Hide outdated src/codegen.cpp
if (li->fptr == &jl_trampoline) {
JL_SIGATOMIC_BEGIN();
#ifdef USE_MCJIT
if (imaging_mode) {

This comment has been minimized.

@vtjnash

vtjnash Jan 3, 2014

Member

emit once, compile twice?

how clever.

I thought shadow module would require emit twice, compile twice. you're other hackery makes much more sense now

@vtjnash

vtjnash Jan 3, 2014

Member

emit once, compile twice?

how clever.

I thought shadow module would require emit twice, compile twice. you're other hackery makes much more sense now

Show outdated Hide outdated src/codegen.cpp
m = jl_Module;
#endif
funcName << globalUnique++;

This comment has been minimized.

@vtjnash

vtjnash Jan 3, 2014

Member

I see that you did deal with the need for globally unique function names

@vtjnash

vtjnash Jan 3, 2014

Member

I see that you did deal with the need for globally unique function names

This comment has been minimized.

@vtjnash

vtjnash Jan 3, 2014

Member

Wouldn't julia_gv need this too?

@vtjnash

vtjnash Jan 3, 2014

Member

Wouldn't julia_gv need this too?

builder.CreateStore(FP(x), builder.CreateBitCast(jlfloattemp_var,FT(x->getType())->getPointerTo()), true);
return builder.CreateFPExt(builder.CreateLoad(builder.CreateBitCast(jlfloattemp_var,FT(x->getType())->getPointerTo()), true),
builder.CreateStore(FP(x), builder.CreateBitCast(prepare_global(jlfloattemp_var),FT(x->getType())->getPointerTo()), true);
return builder.CreateFPExt(builder.CreateLoad(builder.CreateBitCast(prepare_global(jlfloattemp_var),FT(x->getType())->getPointerTo()), true),

This comment has been minimized.

@vtjnash

vtjnash Jan 3, 2014

Member

for some reason, my brain started trying to parse all of the () and -> as anonymous function declarations

this could use some whitespace and rewrapping for readability

@vtjnash

vtjnash Jan 3, 2014

Member

for some reason, my brain started trying to parse all of the () and -> as anonymous function declarations

this could use some whitespace and rewrapping for readability

@vtjnash

This comment has been minimized.

Show comment
Hide comment
@vtjnash

vtjnash Jan 3, 2014

Member

@JeffBezanson @loladiro this looks good to me (ready to merge), from doing a code read-through

Member

vtjnash commented Jan 3, 2014

@JeffBezanson @loladiro this looks good to me (ready to merge), from doing a code read-through

@Keno

This comment has been minimized.

Show comment
Hide comment
@Keno

Keno Jan 13, 2014

Member

Alright, I'll rebase this this and then let's merge it. Actually using MCJIT still crashes sometimes, but to figure out why I need to add more debug info that master can make use of as well and this doesn't change anything for the current REPL.

Member

Keno commented Jan 13, 2014

Alright, I'll rebase this this and then let's merge it. Actually using MCJIT still crashes sometimes, but to figure out why I need to add more debug info that master can make use of as well and this doesn't change anything for the current REPL.

@Keno

This comment has been minimized.

Show comment
Hide comment
@Keno
Member

Keno commented Jan 15, 2014

@Keno

This comment has been minimized.

Show comment
Hide comment
@Keno

Keno Jan 17, 2014

Member

@JeffBezanson Can we just merge this?

Member

Keno commented Jan 17, 2014

@JeffBezanson Can we just merge this?

JeffBezanson added a commit that referenced this pull request Jan 17, 2014

@JeffBezanson JeffBezanson merged commit 43a901f into master Jan 17, 2014

1 check passed

default The Travis CI build passed
Details
@jiahao

This comment has been minimized.

Show comment
Hide comment
@jiahao

jiahao Jan 17, 2014

Member

I bet @loladiro is dancing a jig in his room right now.

Member

jiahao commented Jan 17, 2014

I bet @loladiro is dancing a jig in his room right now.

@johnmyleswhite

This comment has been minimized.

Show comment
Hide comment
@johnmyleswhite

johnmyleswhite Jan 17, 2014

Member

Wheeeeeee!

Member

johnmyleswhite commented Jan 17, 2014

Wheeeeeee!

@ihnorton

This comment has been minimized.

Show comment
Hide comment
@ihnorton

ihnorton Jan 17, 2014

Member

👍

Member

ihnorton commented Jan 17, 2014

👍

@johnmyleswhite johnmyleswhite deleted the kf/mcjit branch Jan 17, 2014

@johnmyleswhite johnmyleswhite restored the kf/mcjit branch Jan 17, 2014

@johnmyleswhite

This comment has been minimized.

Show comment
Hide comment
@johnmyleswhite

johnmyleswhite Jan 17, 2014

Member

Woops. Sorry about the branch deletion. Too excited.

Member

johnmyleswhite commented Jan 17, 2014

Woops. Sorry about the branch deletion. Too excited.

@Keno

This comment has been minimized.

Show comment
Hide comment
@Keno

Keno Jan 17, 2014

Member

I bet @loladiro is dancing a jig in his room right now.

Oh, you betcha

Member

Keno commented Jan 17, 2014

I bet @loladiro is dancing a jig in his room right now.

Oh, you betcha

@StefanKarpinski

This comment has been minimized.

Show comment
Hide comment
@StefanKarpinski

StefanKarpinski Jan 17, 2014

Member

And apparently he's from Fargo too.

Member

StefanKarpinski commented Jan 17, 2014

And apparently he's from Fargo too.

@StefanKarpinski

This comment has been minimized.

Show comment
Hide comment
@StefanKarpinski

StefanKarpinski Jan 17, 2014

Member

So, am I correct that this didn't just add support for MCJIT but also turned on MCJIT as the default JIT?

Member

StefanKarpinski commented Jan 17, 2014

So, am I correct that this didn't just add support for MCJIT but also turned on MCJIT as the default JIT?

@Keno

This comment has been minimized.

Show comment
Hide comment
@Keno

Keno Jan 17, 2014

Member

no, the old JIT is still the default (on 3.3 anyway). This branch was just a pain to maintain, because it touches so many lines

Member

Keno commented Jan 17, 2014

no, the old JIT is still the default (on 3.3 anyway). This branch was just a pain to maintain, because it touches so many lines

@Keno Keno deleted the kf/mcjit branch Aug 10, 2014

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Apr 5, 2015

Coverage Status

Changes Unknown when pulling ed7d660 on kf/mcjit into * on master*.

coveralls commented Apr 5, 2015

Coverage Status

Changes Unknown when pulling ed7d660 on kf/mcjit into * on master*.

@tkelman

This comment has been minimized.

Show comment
Hide comment
@tkelman

tkelman Apr 5, 2015

Contributor

@lemurheavy you guys have a hash collision or something going on, a couple times a month now we get a coveralls notification on a long-closed PR, that judging by the source files (here some python) are for a totally different repo.

Contributor

tkelman commented Apr 5, 2015

@lemurheavy you guys have a hash collision or something going on, a couple times a month now we get a coveralls notification on a long-closed PR, that judging by the source files (here some python) are for a totally different repo.

@mbauman mbauman referenced this pull request May 18, 2015

Merged

tarfix #5567

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment