backport of changes in #17057 that don't require new worlds #18679
Conversation
| end | ||
| end | ||
| for f in fs | ||
| for m in _methods_by_ftype(Tuple{typeof(f), Vararg{Any}}, 10) |
There was a problem hiding this comment.
should this 10 be given a name?
There was a problem hiding this comment.
it's just a random round-ish number
| by < 0 && T <: Unsigned && return false | ||
| ry = rationalize(T,by,tol=0) | ||
| ry < by ? :(x <= $ry) : :(x < $ry) | ||
| @pure function rationalize{T<:Integer}(::Type{T}, x::Irrational) |
There was a problem hiding this comment.
this causes a method overwrite warning, why is it needed?
There was a problem hiding this comment.
for the same reason the other method overwrite warning happens in searchsorted
There was a problem hiding this comment.
any horizon for not needing that?
There was a problem hiding this comment.
The one in this case might not be needed --- calling a kwarg function with no keyword arguments is fast. The problem in searchsorted was that computing the keyword arg default values required dynamic dispatch.
There was a problem hiding this comment.
True, the problem in this case is actually just that lowering won't apply @pure to the kwargs functions, and that's quite fixable.
There was a problem hiding this comment.
add a TODO comment saying to fix pure for kwargs so the no-kwargs method can be deleted then?
There was a problem hiding this comment.
original issue should be corrected by 3f2213f now
| After that, for this example, the expression returned from the generated | ||
| function on the first invocation was re-used as the method body. | ||
| The actual caching behavior is an implementation-defined performance optimization, | ||
| so it is invalid to depend to closely on this behavior, however. |
There was a problem hiding this comment.
"too closely"
don't end a sentence with ", however." - just delete that
|
|
||
| The example generated function ``foo`` above did not do anything a normal | ||
| function ``foo(x)=x*x`` could not do, except printing the type on the | ||
| first invocation (and incurring a higher compile-time cost). However, the |
There was a problem hiding this comment.
why the rewording from "compile-time cost" to "overhead" ? is this not strictly a compile time difference?
There was a problem hiding this comment.
no, it's adds overhead in many places
| function ``foo(x) = x * x`` could not do (except printing the type on the | ||
| first invocation, and incurring higher overhead). | ||
| However, the power of a generated function lies in its ability to compute | ||
| different quoted expression depending on the types passed to it: |
There was a problem hiding this comment.
different quoted expressions
| It's easiest to illustrate this with an example. We can declare a generated | ||
| function ``foo`` as | ||
|
|
||
| .. doctest:: | ||
|
|
||
| julia> @generated function foo(x) | ||
| println(x) | ||
| return :(x*x) | ||
| Core.println(x) |
There was a problem hiding this comment.
But this is not pure. This is mentioned way later, but coming after the new point "4." this is just odd. Maybe state that doing println is ok?
There was a problem hiding this comment.
println isn't OK, it's just a convenient function for showing how they work (and along the way demonstrating why it isn't OK)
|
|
||
| 3. Instead of calculating something or performing some action, you return | ||
| a *quoted expression* which, when evaluated, does what you want. | ||
|
|
||
| 4. The function must not have any side-effects or examine any non-constant |
There was a problem hiding this comment.
To go with the later "they"s, start with "4. Generated functions must not..."
| ordinary functions: | ||
|
|
||
| 1. You annotate the function declaration with the ``@generated`` macro. | ||
| This adds some information to the AST that lets the compiler know that | ||
| this is a generated function. | ||
|
|
||
| 2. In the body of the generated function you only have access to the | ||
| *types* of the arguments, not their values. | ||
| *types* of the arguments -- not their values -- and any function that | ||
| was defined *before* the definition of generated function. |
There was a problem hiding this comment.
"was defined before the definition of the generated function."
| @@ -1028,12 +1037,40 @@ copy them*, for the following reasons: | |||
| * the ``foo`` function has side-effects, and it is undefined exactly when, | |||
There was a problem hiding this comment.
As @mauro3 suggested in https://github.com/JuliaLang/julia/pull/18679/files#r80905058, perhaps also mention the issues immediately alongside the corresponding examples?
| This condition is relaxed for incrementally-loaded precompiled modules to | ||
| allow calling any function in the module. | ||
|
|
||
| Alright, now that we have a better understanding for how generated functions |
There was a problem hiding this comment.
for -> of ("now that we have a better understanding of how generated functions work").
| @@ -1034,7 +1034,7 @@ These examples are hopefully helpful to illustrate how generated functions | |||
| work, both in the definition end and at the call site; however, *don't | |||
| copy them*, for the following reasons: | |||
|
|
|||
| * the ``foo`` function has side-effects, and it is undefined exactly when, | |||
| * the ``foo`` function has side-effects (the call to `Core.println`), and it is undefined exactly when, | |||
vtjnash
force-pushed
the
jn/backports265.1
branch
from
8f4d569
to
a72753e
Compare
the widenconst for slottypes happens before optimization, so need to be careful not to introduce `Const` in that array during optimization
…unctions from irrationals
vtjnash
force-pushed
the
jn/backports265.1
branch
from
a72753e
to
d790275
Compare
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
|
nanosoldier says, "merge"? |
| @@ -829,7 +829,7 @@ static jl_method_instance_t *jl_mt_assoc_by_type(jl_methtable_t *mt, jl_datatype | |||
| sig = join_tsig(tt, entry->sig); | |||
| jl_method_instance_t *nf; | |||
| if (!cache) { | |||
| nf = jl_get_specialized(m, sig, env); // TODO: should be jl_specializations_get_linfo | |||
| nf = jl_specializations_get_linfo(m, sig, env); | |||
There was a problem hiding this comment.
Just FYI, this change doesn't fix the ASAN issue I mentioned.
|
I'm excited for the new worlds to come. |
Fixes some method definition orderings, and other
@generatedfunctions, that will be broken by PR #17057