Skip to content
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

some fixes for compilation #27898

Merged
merged 5 commits into from
Jul 4, 2018
Merged

some fixes for compilation #27898

merged 5 commits into from
Jul 4, 2018

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Jul 2, 2018

No description provided.

@ararslan ararslan added the compiler:codegen Generation of LLVM IR and native code label Jul 2, 2018
@@ -601,7 +601,7 @@ static void jl_compilation_sig(
for (i = 0; i < np; i++) {
jl_value_t *elt = jl_tparam(tt, i);
jl_value_t *decl_i = jl_nth_slot_type(decl, i);
size_t i_arg = (i <= nargs ? i : nargs);
size_t i_arg = (i < nargs - 1 ? i : nargs - 1);
Copy link
Member

Choose a reason for hiding this comment

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

min(i, nargs - 1) for readability?

Copy link
Member Author

Choose a reason for hiding this comment

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

Readability+C? You're funny. The min function can't exist in C since it doesn't have templates, and fmin is a floating point function.

src/gf.c Outdated
if (definition->isva ? np <= nargs : np != nargs)
return 0;
if (np != nargs) {
if (!definition->isva || (np <= nargs && jl_is_vararg_type(jl_tparam(type, np - 1)))) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite understand this condition. Why does the vararg matter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only Vararg functions may be compiled for Vararg signatures

Copy link
Member

Choose a reason for hiding this comment

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

Right, but isn't this asking the opposite?

Copy link
Member Author

@vtjnash vtjnash Jul 3, 2018

Choose a reason for hiding this comment

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

This (should?) check the condition up to the "only" clause. We should also check for the rest of that somewhere, but currently appear to be probably missing that check. It's not particularly important.

Copy link
Member

Choose a reason for hiding this comment

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

Right, so this disallows anything of the form say Tuple{typeof(f), Int...}, for say f(a::Int, b::Int...) which may be what you intended, but I thought you may have intended the opposite.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, this check is for ensure that we have enough parameters to form a Vararg. For np == nargs, we expect to just compile a concrete signature

@vtjnash vtjnash force-pushed the jn/codegen-fixes branch from 2e7620d to 11a8c25 Compare July 3, 2018 15:32
@vtjnash vtjnash force-pushed the jn/codegen-fixes branch from b3f9256 to 01d8f80 Compare July 3, 2018 23:30
@vtjnash vtjnash merged commit a726763 into master Jul 4, 2018
@vtjnash vtjnash deleted the jn/codegen-fixes branch July 4, 2018 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:codegen Generation of LLVM IR and native code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants