-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
replace @pure
annotations in Base with effect settings
#44776
Conversation
macro _pure_meta() | ||
return Expr(:meta, :pure) | ||
end | ||
# can be used in place of `@assume_effects :total` (supposed to be used for bootstrapping) | ||
macro _total_meta() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe name this _effect_total
? The current name seems fairly confusing.
@@ -151,9 +151,29 @@ macro isdefined(s::Symbol) | |||
return Expr(:escape, Expr(:isdefined, s)) | |||
end | |||
|
|||
# can be used in place of `@pure` (supposed to be used for bootstrapping) | |||
macro _pure_meta() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we delete _pure_meta
now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and @pure
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's technically internal so we should be able to do so, but I suspect there's a decent amount of package code that uses it, so we should probably at least go through some kind of deprecation process. Or maybe just make PRs to affected packages to not use it if VERSION
≥ v"1.9".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also be sneaky and make it a no-op and then see if people actually notice...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is anyone using it correctly? I don't think @vtjnash has added uses to packages, and as far as I understand, he is the only person allowed to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now all of these internal macros are no-op when used outside of Base ;)
base/reflection.jl
Outdated
(!isvarargtype(a) && !isvarargtype(b)) || throw(ArgumentError("invalid typeintersect with Vararg")) | ||
return ccall(:jl_type_intersection, Any, (Any, Any), a, b) | ||
# equivalent to: @assume_effects :total @ccall jl_type_intersection(a::Any, b::Any)::Any | ||
return $(Expr(:foreigncall, QuoteNode(:jl_type_intersection), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How feasible would it be to define the ccall macro earlier or maybe have a limited version of it. I was fine with one or two of those, but if this expands, it's a bit of a nightmare.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might even be worth making it a non-proper scheme macro (not handled by macroexpand), since it can be rather complex to unpack the various possible arguments, but therefore also worthwhile to have very early in bootstrapping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent a bit time to rearrange utilities of essentials.jl/reflection.jl/expr.jl, but didn't succeed to bootstrap. The most difficult part might be that operators.jl isn't available at a time of using @ccall
in reflection.jl.
For these specific utilities, I think we can use @_total_but_may_throw_meta
instead and it won't be that bad.
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
@@ -48,7 +48,8 @@ AbstractFloat(x::AbstractIrrational) = Float64(x)::Float64 | |||
Float16(x::AbstractIrrational) = Float16(Float32(x)::Float32) | |||
Complex{T}(x::AbstractIrrational) where {T<:Real} = Complex{T}(T(x)) | |||
|
|||
@pure function Rational{T}(x::AbstractIrrational) where T<:Integer | |||
# XXX this may change `DEFAULT_PRECISION`, thus not effect free |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
horribly, the return value also includes novel memory allocation and depends on the global runtime state of the DEFAULT_PRECISION
|
This commit adds new reflection utility named `Base.infer_effects` that works in the same way as `Base.return_types` but returns inferred effects instead. It would be helpful to test that certain method call has an expected effects. For example, we can now remove `Base.@pure` annotation from the definition of `BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}` and checks it's still eligible for concrete evaluation like this (see <#44776 (comment)> for the context): ```julia julia> import Base.Broadcast: AbstractArrayStyle, DefaultArrayStyle, Unknown julia> function BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N} if Base.typename(A) === Base.typename(B) return A(Val(max(M, N))) end return Unknown() end BroadcastStyle (generic function with 1 method) julia> # test that the above definition is eligible for concrete evaluation @test Base.infer_effects(BroadcastStyle, (DefaultArrayStyle{1},DefaultArrayStyle{2},)) |> Core.Compiler.is_total_or_error Test Passed ```
This commit adds new reflection utility named `Base.infer_effects` that works in the same way as `Base.return_types` but returns inferred effects instead. It would be helpful to test that certain method call has an expected effects. For example, we can now remove `Base.@pure` annotation from the definition of `BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}` and checks it's still eligible for concrete evaluation like this (see <#44776 (comment)> for the context): ```julia julia> import Base.Broadcast: AbstractArrayStyle, DefaultArrayStyle, Unknown julia> function BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N} if Base.typename(A) === Base.typename(B) return A(Val(max(M, N))) end return Unknown() end BroadcastStyle (generic function with 1 method) julia> # test that the above definition is eligible for concrete evaluation @test Base.infer_effects(BroadcastStyle, (DefaultArrayStyle{1},DefaultArrayStyle{2},)) |> Core.Compiler.is_total_or_error Test Passed ```
This commit adds new reflection utility named `Base.infer_effects` that works in the same way as `Base.return_types` but returns inferred effects instead. It would be helpful to test that certain method call has an expected effects. For example, we can now remove `Base.@pure` annotation from the definition of `BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}` and checks it's still eligible for concrete evaluation like this (see <#44776 (comment)> for the context): ```julia julia> import Base.Broadcast: AbstractArrayStyle, DefaultArrayStyle, Unknown julia> function BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N} if Base.typename(A) === Base.typename(B) return A(Val(max(M, N))) end return Unknown() end BroadcastStyle (generic function with 1 method) julia> # test that the above definition is eligible for concrete evaluation @test Base.infer_effects(BroadcastStyle, (DefaultArrayStyle{1},DefaultArrayStyle{2},)) |> Core.Compiler.is_total_or_error Test Passed ```
fdf30db
to
16aabfa
Compare
52400d9
to
7865fc2
Compare
16aabfa
to
4e68d57
Compare
7865fc2
to
5bd710a
Compare
I'd conclude this is not a regression. This benchmark is actually fully constant folded and so the difference may just be a noise. This benchmark is equivalent to the following code snippet, which would be fully folded both on master and this PR: julia> t1 = (1,2,3)
(1, 2, 3)
julia> @eval code_typed() do
broadcast(+, 1, $t1, 1, $t1, 1, $t1)
end
1-element Vector{Any}:
CodeInfo(
1 ─ return (6, 9, 12)
) => Tuple{Int64, Int64, Int64} |
5bd710a
to
506e444
Compare
This commit adds new reflection utility named `Base.infer_effects` that works in the same way as `Base.return_types` but returns inferred effects instead. It would be helpful to test that certain method call has an expected effects. For example, we can now remove `Base.@pure` annotation from the definition of `BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}` and checks it's still eligible for concrete evaluation like this (see <#44776 (comment)> for the context): ```julia julia> import Base.Broadcast: AbstractArrayStyle, DefaultArrayStyle, Unknown julia> function BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N} if Base.typename(A) === Base.typename(B) return A(Val(max(M, N))) end return Unknown() end BroadcastStyle (generic function with 1 method) julia> # test that the above definition is eligible for concrete evaluation @test Base.infer_effects(BroadcastStyle, (DefaultArrayStyle{1},DefaultArrayStyle{2},)) |> Core.Compiler.is_total_or_error Test Passed ```
This commit adds new reflection utility named `Base.infer_effects` that works in the same way as `Base.return_types` but returns inferred effects instead. It would be helpful to test that certain method call has an expected effects. For example, we can now remove `Base.@pure` annotation from the definition of `BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}` and checks it's still eligible for concrete evaluation like this (see <#44776 (comment)> for the context): ```julia julia> import Base.Broadcast: AbstractArrayStyle, DefaultArrayStyle, Unknown julia> function BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N} if Base.typename(A) === Base.typename(B) return A(Val(max(M, N))) end return Unknown() end BroadcastStyle (generic function with 1 method) julia> # test that the above definition is eligible for concrete evaluation @test Base.infer_effects(BroadcastStyle, (DefaultArrayStyle{1},DefaultArrayStyle{2},)) |> Core.Compiler.is_total_or_error Test Passed ```
This commit adds new reflection utility named `Base.infer_effects` that works in the same way as `Base.return_types` but returns inferred effects instead. It would be helpful to test that certain method call has an expected effects. For example, we can now remove `Base.@pure` annotation from the definition of `BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}` and checks it's still eligible for concrete evaluation like this (see <#44776 (comment)> for the context): ```julia julia> import Base.Broadcast: AbstractArrayStyle, DefaultArrayStyle, Unknown julia> function BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N} if Base.typename(A) === Base.typename(B) return A(Val(max(M, N))) end return Unknown() end BroadcastStyle (generic function with 1 method) julia> # test that the above definition is eligible for concrete evaluation @test Base.infer_effects(BroadcastStyle, (DefaultArrayStyle{1},DefaultArrayStyle{2},)) |> Core.Compiler.is_total_or_error Test Passed ```
This commit adds new reflection utility named `Base.infer_effects` that works in the same way as `Base.return_types` but returns inferred effects instead. It would be helpful to test that certain method call has an expected effects. For example, we can now remove `Base.@pure` annotation from the definition of `BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}` and checks it's still eligible for concrete evaluation like this (see <#44776 (comment)> for the context): ```julia julia> import Base.Broadcast: AbstractArrayStyle, DefaultArrayStyle, Unknown julia> function BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N} if Base.typename(A) === Base.typename(B) return A(Val(max(M, N))) end return Unknown() end BroadcastStyle (generic function with 1 method) julia> # test that the above definition is eligible for concrete evaluation @test Base.infer_effects(BroadcastStyle, (DefaultArrayStyle{1},DefaultArrayStyle{2},)) |> Core.Compiler.is_total_or_error Test Passed ```
This commit adds new reflection utility named `Base.infer_effects` that works in the same way as `Base.return_types` but returns inferred effects instead. It would be helpful to test that certain method call has an expected effects. For example, we can now remove `Base.@pure` annotation from the definition of `BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}` and checks it's still eligible for concrete evaluation like this (see <#44776 (comment)> for the context): ```julia julia> import Base.Broadcast: AbstractArrayStyle, DefaultArrayStyle, Unknown julia> function BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N} if Base.typename(A) === Base.typename(B) return A(Val(max(M, N))) end return Unknown() end BroadcastStyle (generic function with 1 method) julia> # test that the above definition is eligible for concrete evaluation @test Base.infer_effects(BroadcastStyle, (DefaultArrayStyle{1},DefaultArrayStyle{2},)) |> Core.Compiler.is_total_or_error Test Passed ```
This commit adds new reflection utility named `Base.infer_effects` that works in the same way as `Base.return_types` but returns inferred effects instead. It would be helpful to test that certain method call has an expected effects. For example, we can now remove `Base.@pure` annotation from the definition of `BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}` and checks it's still eligible for concrete evaluation like this (see <#44776 (comment)> for the context): ```julia julia> import Base.Broadcast: AbstractArrayStyle, DefaultArrayStyle, Unknown julia> function BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N} if Base.typename(A) === Base.typename(B) return A(Val(max(M, N))) end return Unknown() end BroadcastStyle (generic function with 1 method) julia> # test that the above definition is eligible for concrete evaluation @test Base.infer_effects(BroadcastStyle, (DefaultArrayStyle{1},DefaultArrayStyle{2},)) |> Core.Compiler.is_total_or_error Test Passed ``` Co-authored-by: Takafumi Arakaki <[email protected]>
This commit adds new reflection utility named `Base.infer_effects` that works in the same way as `Base.return_types` but returns inferred effects instead. It would be helpful to test that certain method call has an expected effects. For example, we can now remove `Base.@pure` annotation from the definition of `BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}` and checks it's still eligible for concrete evaluation like this (see <#44776 (comment)> for the context): ```julia julia> import Base.Broadcast: AbstractArrayStyle, DefaultArrayStyle, Unknown julia> function BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N} if Base.typename(A) === Base.typename(B) return A(Val(max(M, N))) end return Unknown() end BroadcastStyle (generic function with 1 method) julia> # test that the above definition is eligible for concrete evaluation @test Base.infer_effects(BroadcastStyle, (DefaultArrayStyle{1},DefaultArrayStyle{2},)) |> Core.Compiler.is_total_or_error Test Passed ``` Co-authored-by: Takafumi Arakaki <[email protected]>
4fd0b88
to
91bef41
Compare
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
91bef41
to
659bfe8
Compare
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
Okay, only the I confirmed that there seems to be no regression in the diff --git a/base/compiler/abstractinterpretation.jl b/base/compiler/abstractinterpretation.jl
index 0cca9c38d2..5d65895a28 100644
--- a/base/compiler/abstractinterpretation.jl
+++ b/base/compiler/abstractinterpretation.jl
@@ -676,7 +676,17 @@ function pure_eval_eligible(interp::AbstractInterpreter,
# XXX we need to check that this pure function doesn't call any overlayed method
return f !== nothing &&
length(applicable) == 1 &&
- is_method_pure(applicable[1]::MethodMatch) &&
+ begin
+ match = applicable[1]::MethodMatch
+ is_method_pure(match) || begin
+ if isoverlayed(method_table(interp))
+ false
+ else
+ effects = decode_effects_override(match.method.purity)
+ effects.consistent && effects.effect_free && effects.terminates_globally
+ end
+ end
+ end &&
is_all_const_arg(arginfo)
end I'd rather accept that slight regression for now though, as I believe we will focus our development efforts on the new effects analysis rather than putting more on the |
659bfe8
to
0953fa9
Compare
This commit replaces `@pure`/`@_pure_meta` annotations used in `Base` with corresponding effect settings (`:total` or `:total_or_throw`). The concrete evaluation mechanism based on the effect system (#43852) has the following benefits over the `@pure`-based optimization: - it can fold cases when consistent exception is thrown - it can handle constant union-split situation as well - effects can be propagated inter-procedurally While revisiting the existing annotations, I removed some unnecessary ones and also added some more hopefully new annotations (mostly for reflection utilities). In theory this should give us some performance benefits, e.g. now we can concrete-evaluate union-spit `typeintersect`: ```julia @test Base.return_types((Union{Int,Nothing},)) do x typeintersect(String, typeof(x)) end |> only === Type{Union{}} ``` Though this commit ends up bigger than I expected -- we should carefully check a benchmark result so that it doesn't come with any regressions.
We discussed that for certain methods like `:total`ly-declared method or `@nospecialize`d method we may want to only do constant propagation while disabling preceding only-type-level inference. This commit implements a simple infrastructure for such early constant propagation, and especially setups early concrete evaluation pass. This commit should recover the regression reported at <#44776 (comment)> while preserving the advantages and correctness improvements of the `@assume_effects`-based concrete evaluation enabled in the PR.
0953fa9
to
cdb7c37
Compare
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
We discussed that for certain methods like `:total`ly-declared method or `@nospecialize`d method we may want to only do constant propagation while disabling preceding only-type-level inference. This commit implements a simple infrastructure for such early constant propagation, and especially setups early concrete evaluation pass. This commit should recover the regression reported at <#44776 (comment)> while preserving the advantages and correctness improvements of the `@assume_effects`-based concrete evaluation enabled in the PR.
We discussed that for certain methods like `:total`ly-declared method or `@nospecialize`d method we may want to only do constant propagation while disabling preceding only-type-level inference. This commit implements a simple infrastructure for such early constant propagation, and especially setups early concrete evaluation pass. This commit should recover the regression reported at <#44776 (comment)> while preserving the advantages and correctness improvements of the `@assume_effects`-based concrete evaluation enabled in the PR.
I was doing a little hacking on inference and noticed that julia> include("compiler/compiler.jl")
WARNING: replacing module Compiler.
ERROR: LoadError: UndefVarError: ntuple not defined
Stacktrace:
[1] getindex(t::Tuple, r::Core.Compiler.UnitRange{Int64})
@ Core.Compiler ./range.jl:414
[2] var"@assume_effects"(__source__::LineNumberNode, __module__::Module, args::Vararg{Any})
@ Core.Compiler ./expr.jl:628
[3] include
@ ./boot.jl:368 [inlined]
[4] include(x::String)
@ Core.Compiler ~/src/julia-master/base/compiler/compiler.jl:23
[5] top-level scope
@ ~/src/julia-master/base/compiler/compiler.jl:93
[6] eval(m::Module, e::Any)
@ Core ./boot.jl:370
[7] top-level scope
@ ~/src/julia-master/base/compiler/compiler.jl:3
[8] include(fname::String)
@ Base.MainInclude ./client.jl:472
[9] top-level scope
@ REPL[1]:1
in expression starting at namedtuple.jl:221
in expression starting at namedtuple.jl:221
in expression starting at /home/tim/src/julia-master/base/compiler/compiler.jl:3 traces to a line in |
Should be fixed by #45849. |
This commit replaces
@pure
/@_pure_meta
annotations used inBase
with corresponding effect settings (
:total
or:total_or_throw
).The concrete evaluation mechanism based on the effect system (#43852)
has the following benefits over the
@pure
-based optimization:While revisiting the existing annotations, I removed some unnecessary ones
and also added some more hopefully helpful new annotations (mostly for reflection utilities).
In theory this should give us some performance benefits, e.g. now we
can concrete-evaluate union-spit
typeintersect
:Though this commit ends up bigger than I expected -- we should carefully
check a benchmark result so that it doesn't come with any regressions.
@nanosoldier
runbenchmarks(!"scalar", vs=":master")