Skip to content

Commit

Permalink
(broken) patch to eliminate emit_func_check
Browse files Browse the repository at this point in the history
  • Loading branch information
@stevengj
stevengj committed Aug 14, 2014
1 parent 0df037e commit 762a727
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 31 deletions.
23 changes: 0 additions & 23 deletions src/cgutils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -824,29 +824,6 @@ static Value *emit_bounds_check(Value *i, Value *len, jl_codectx_t *ctx)
return im1;
}

static void emit_func_check(Value *x, jl_codectx_t *ctx)
{
Value *xty = emit_typeof(x);
Value *isfunc =
builder.
CreateOr(builder.
CreateICmpEQ(xty,
literal_pointer_val((jl_value_t*)jl_function_type)),
builder.
CreateICmpEQ(xty,
literal_pointer_val((jl_value_t*)jl_datatype_type)));
BasicBlock *elseBB1 = BasicBlock::Create(getGlobalContext(),"notf", ctx->f);
BasicBlock *mergeBB1 = BasicBlock::Create(getGlobalContext(),"isf");
builder.CreateCondBr(isfunc, mergeBB1, elseBB1);

builder.SetInsertPoint(elseBB1);
emit_type_error(x, (jl_value_t*)jl_function_type, "apply", ctx);

builder.CreateBr(mergeBB1);
ctx->f->getBasicBlockList().push_back(mergeBB1);
builder.SetInsertPoint(mergeBB1);
}

// --- loading and storing ---

static Value *emit_nthptr_addr(Value *v, size_t n)
Expand Down
14 changes: 6 additions & 8 deletions src/codegen.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2219,8 +2219,12 @@ static Value *emit_call(jl_value_t **args, size_t arglen, jl_codectx_t *ctx,
if (theFptr == NULL) {
specialized = false;
Value *theFunc = emit_expr(args[0], ctx);

This comment has been minimized.

Sign in to view

Copy link
@stevengj

stevengj Aug 14, 2014

Author Member

This theFunc = emit_expr(...) can probably be moved into the else clause, since presumably theFunc->getType() != jl_pvalue_llvmt will always be false if hdtype==(jl_value_t*)jl_function_type?
if (theFunc->getType() != jl_pvalue_llvmt || jl_is_tuple(hdtype)) {
// we know it's not a function
if ((hdtype!=(jl_value_t*)jl_function_type &&
hdtype!=(jl_value_t*)jl_datatype_type &&
!(jl_is_type_type(hdtype) &&
jl_is_datatype(jl_tparam0(hdtype)))) ||
jl_is_tuple(hdtype) || theFunc->getType() != jl_pvalue_llvmt) {
// it may not be a function, use call(f, ...) instead

This comment has been minimized.

Sign in to view

Copy link
@vtjnash

vtjnash Aug 14, 2014

Member

the issue is probably that this depends too heavily on type-inference, so it tries to turn everything into a call(f,...), until inference gets a chance to start running. might be necessary. i think this means you will need to always emit the emit_func_check branch here, just changing the else clause. it's probably better to be avoiding the tuple pack/unpack anyways.

This comment has been minimized.

Sign in to view

Copy link
@vtjnash

vtjnash Aug 15, 2014

Member

after thinking about this some more, i'm not sure if you want this here at all. perhaps, the branch should be handled by jl_apply_generic? in either case, it needs a custom lowering pass in inlining_pass, similar to ^ and apply

This comment has been minimized.

Sign in to view

Copy link
@stevengj

stevengj Aug 15, 2014

Author Member

I can understand why this might not be optimal, but why would turning lots of things into call(f, ...) cause an undefined-variable error? ... oh, I see, the tuple unpacking.

This comment has been minimized.

Sign in to view

Copy link
@jakebolewski

jakebolewski Aug 15, 2014

Member

@vtjnash at the point where it is failing in building the sysimg (Int.jl), shouldn't the tuple start method already be defined (it preceeds int.jl in build order)? I did some debugging after you left this afternoon @stevengj (replacating pretty much what @vtjnash posted) but saw that it was failing in eval with start undefined, and to me this didn't make any sense as the start method exists for tuples at that point (or it doesn't and I'm just misunderstanding how the sysimg build process works).

This comment has been minimized.

Sign in to view

Copy link
@vtjnash

vtjnash Aug 15, 2014

Member

That's a good point. In fact, start is never defined, and thus ... can't be used inside of Core, ever. (well, without first defining Core.start)

julia> Core.start
ERROR: start not defined

This comment has been minimized.

Sign in to view

Copy link
@vtjnash

vtjnash Aug 15, 2014

Member

oh, I see, the tuple unpacking.

i see my comments aren't very well connected. this one was supposed to be first:
#8008 (comment)
headIsGlobal = true;
Value *result = emit_known_call((jl_value_t*)jl_call_func,
--args, ++nargs, ctx,
Expand All @@ -2234,12 +2238,6 @@ static Value *emit_call(jl_value_t **args, size_t arglen, jl_codectx_t *ctx,
make_gcroot(boxed(theFunc,ctx), ctx);
}
#endif
if (hdtype!=(jl_value_t*)jl_function_type &&
hdtype!=(jl_value_t*)jl_datatype_type &&
!(jl_is_type_type(hdtype) &&
jl_is_datatype(jl_tparam0(hdtype)))) {
emit_func_check(theFunc, ctx);
}
// extract pieces of the function object
// TODO: try extractvalue instead
theFptr = builder.CreateBitCast(emit_nthptr(theFunc, 1, tbaa_func), jl_fptr_llvmt);
Expand Down

3 comments on commit 762a727

@stevengj
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what is going on here. After this patch, make clean && make gives

int.jl
error during bootstrap: LoadError(at "sysimg.jl" line 42: LoadError(at "int.jl" line 280: UndefVarError(var=:start)))

@JeffBezanson or @vtjnash, any tips would be welcome.

@vtjnash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The --args, ++nargs seems suspicious, but I can't fully review that from a phone

@stevengj
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of the code only accesses args[1] or later, so this just shifts the pointer to make args[0] the first argument.

Also, that part of the code seems to be working...it was used in the previous commit to implement call overloading, and worked fine until this patch.

Please sign in to comment.