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

Force compilation with @force_compile #42379

Merged
merged 1 commit into from
Oct 22, 2021
Merged

Force compilation with @force_compile #42379

merged 1 commit into from
Oct 22, 2021

Conversation

timholy
Copy link
Member

@timholy timholy commented Sep 25, 2021

There was an unintended collision in meaning between #42128 and #37041.
Since both use the Expr(:meta) mechanism, it doesn't really seem
feasible to have them both use the same name. Consequently, it's
better to rename the newer meaning.

Fixes #42373

@vchuravy
Copy link
Member

Maybe ensure_compiled or assert_compiled? Reads more natural for me.

@KristofferC
Copy link
Member

KristofferC commented Sep 25, 2021

I don't understand where the actual collision occurs though. The meta from compile=min gets set on the module while the @compile gets set on the method and the retrieval of this info doesn't seem like they would mix them up.

Also, there is no need to change the macro name, right? Just what meta symbol gets injected. But perhaps force_compile is more descriptive.

@timholy
Copy link
Member Author

timholy commented Sep 25, 2021

julia> macroexpand(Main, :(Base.Experimental.@compiler_options compile=min optimize=0 infer=false))
quote
    $(Expr(:meta, :compile, 3))
    $(Expr(:meta, :optlevel, 0))
    $(Expr(:meta, :infer, 0))
end

So what happens:

  • toplevel scanning gets Exprs first. It detects a :compile symbol. The new code says, "OK, let's compile this"
  • so it gets compiled, which generates nice, efficient code for creating a couple of Exprs. Which then get garbage collected, because...
  • ...the interpreter is needed to set the compiler options:

    julia/src/interpreter.c

    Lines 556 to 581 in 53aa65a

    else if (head == meta_sym) {
    if (jl_expr_nargs(stmt) == 1 && jl_exprarg(stmt, 0) == (jl_value_t*)nospecialize_sym) {
    jl_set_module_nospecialize(s->module, 1);
    }
    if (jl_expr_nargs(stmt) == 1 && jl_exprarg(stmt, 0) == (jl_value_t*)specialize_sym) {
    jl_set_module_nospecialize(s->module, 0);
    }
    if (jl_expr_nargs(stmt) == 2) {
    if (jl_exprarg(stmt, 0) == (jl_value_t*)optlevel_sym) {
    if (jl_is_long(jl_exprarg(stmt, 1))) {
    int n = jl_unbox_long(jl_exprarg(stmt, 1));
    jl_set_module_optlevel(s->module, n);
    }
    }
    else if (jl_exprarg(stmt, 0) == (jl_value_t*)compile_sym) {
    if (jl_is_long(jl_exprarg(stmt, 1))) {
    jl_set_module_compile(s->module, jl_unbox_long(jl_exprarg(stmt, 1)));
    }
    }
    else if (jl_exprarg(stmt, 0) == (jl_value_t*)infer_sym) {
    if (jl_is_long(jl_exprarg(stmt, 1))) {
    jl_set_module_infer(s->module, jl_unbox_long(jl_exprarg(stmt, 1)));
    }
    }
    }
    }

@vtjnash
Copy link
Member

vtjnash commented Sep 25, 2021

the interpreter is needed to set the compiler options

That sounds like an additional bug. Normally codegen will execute statements of this type in the interpreter.

@timholy
Copy link
Member Author

timholy commented Sep 25, 2021

Right, but #42128 made anything with an Expr(:meta, :compile, ...) be force-compiled, not realizing that already had an almost opposite meaning.

@codecov
Copy link

codecov bot commented Sep 26, 2021

Codecov Report

Merging #42379 (53aa65a) into master (792c026) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 53aa65a differs from pull request most recent head 74a55dc. Consider uploading reports for the commit 74a55dc to get more accurate results
Impacted file tree graph

@@           Coverage Diff            @@
##           master   #42379    +/-   ##
========================================
  Coverage   89.32%   89.33%            
========================================
  Files         343      343            
  Lines       79668    79435   -233     
========================================
- Hits        71163    70961   -202     
+ Misses       8505     8474    -31     
Impacted Files Coverage Δ
base/gcutils.jl 90.90% <0.00%> (-5.76%) ⬇️
base/secretbuffer.jl 96.84% <0.00%> (-3.16%) ⬇️
base/compiler/ssair/inlining.jl 90.56% <0.00%> (-2.81%) ⬇️
stdlib/LinearAlgebra/src/factorization.jl 85.50% <0.00%> (-1.45%) ⬇️
stdlib/LinearAlgebra/src/qr.jl 95.85% <0.00%> (-1.15%) ⬇️
base/parse.jl 87.26% <0.00%> (-0.95%) ⬇️
base/bitset.jl 93.03% <0.00%> (-0.82%) ⬇️
base/version.jl 84.17% <0.00%> (-0.72%) ⬇️
base/iostream.jl 85.56% <0.00%> (-0.69%) ⬇️
base/float.jl 87.19% <0.00%> (-0.64%) ⬇️
... and 69 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20e8f46...74a55dc. Read the comment docs.

@KristofferC
Copy link
Member

Maybe ensure_compiled or assert_compiled?

I think the current name is the most descriptive. assert_compiled sounds like something "checking" i.e. something that would error in case this block did not end up compiled, but not something that "forces" the block to be compiled.

@KristofferC
Copy link
Member

KristofferC commented Sep 26, 2021

Would it be "cleaner" to piggyback on the compiler_options=:yes meta node which already has a meaning:

#a === :yes ? 1 :

So @force_compile would put in a Expr(:meta, :compile, 1) and instead of only checking if a meta exists, we also check that it has the right value. That seems like it would solve the original issue as well. And then in the future you could envision being able to do compile=min on individual methods etc. So having these use the same meta node might in the end be advantageous.

@timholy
Copy link
Member Author

timholy commented Sep 27, 2021

But currently won't that affect the compile setting module-wide? How do you distinguish it from something that is intended to be passed to jl_set_module_optlevel?

@KristofferC
Copy link
Member

KristofferC commented Sep 27, 2021

I thought the difference would be whether the annotation is made on top-level (affects the module) or on a method (only affects the method).

@timholy
Copy link
Member Author

timholy commented Sep 27, 2021

No, for example with @time it's all at top-level.

@KristofferC
Copy link
Member

Hm, I thought I gave the @time example in my post but seems I deleted it...:

image

Anyway, yes, makes sense that they probably have to be different, indeed.

@JeffBezanson
Copy link
Member

I think the version that affects the whole module should not be allowed inside a method, and then that will help disambiguate a bit. We can also use e.g. Expr(:meta, :compile, :force). I think it helps to have fewer different forms. Also the surface syntax is totally independent and can stay @compile. It can also be moved to base/experimental if we're still not sure of the API.

timholy added a commit that referenced this pull request Sep 28, 2021
This more explicit directive distinguishes `@compile`
(now moved to Experimental) from `@compiler_options`
(which uses the same `:compile` symbol). It eliminates
a collision between #42128 and #37041.

Fixes #42373
Closes #42379
@timholy
Copy link
Member Author

timholy commented Sep 28, 2021

xref #42406. I didn't fix the "not allowed inside a method" for #37401. I liked the idea of moving it to Experimental, so I did that.

@JeffBezanson
Copy link
Member

Rebased.

@timholy
Copy link
Member Author

timholy commented Oct 21, 2021

Thanks (it's been a busy month or so). Anything else needed here? Should I move this to Experimental?

There was an unintended collision in meaning between #42128 and #37041.
Since both use the `Expr(:meta)` mechanism, it doesn't really seem
feasible to have them both use the same name. Consequently, it's
better to rename the newer meaning.

Fixes #42373
@timholy timholy merged commit 835cd0f into master Oct 22, 2021
@timholy timholy deleted the teh/fix_42373 branch October 22, 2021 00:51
=#

if Sys.iswindows()
Experimental.@compile
Copy link
Member

Choose a reason for hiding this comment

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

Is it

Suggested change
Experimental.@compile
Experimental.@force_compile

here?

Copy link
Member

Choose a reason for hiding this comment

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

The iswindows check feels kind of artificial here as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch both of you. See #42760

LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
There was an unintended collision in meaning between JuliaLang#42128 and JuliaLang#37041.
Since both use the `Expr(:meta)` mechanism, it doesn't really seem
feasible to have them both use the same name. Consequently, it's
better to rename the newer meaning.

Fixes JuliaLang#42373
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
There was an unintended collision in meaning between JuliaLang#42128 and JuliaLang#37041.
Since both use the `Expr(:meta)` mechanism, it doesn't really seem
feasible to have them both use the same name. Consequently, it's
better to rename the newer meaning.

Fixes JuliaLang#42373
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slowdown in loading packages on master
6 participants