-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
effects: power-up effects analysis for array operations #47154
Conversation
85598c2
to
6be923c
Compare
@nanosoldier |
6be923c
to
58294d3
Compare
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
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.
SGTM overall, with questions/comments
58294d3
to
ead7b17
Compare
ead7b17
to
1f57fe9
Compare
71d4e32
to
7ad158f
Compare
Mostly by making use of newly added `:inaccessiblememonly` effect property. Now we can fold simple vector operations like: ```julia julia> function simple_vec_ops(T, op!, op, xs...) a = T[] op!(a, xs...) return op(a) end; simple_vec_ops (generic function with 1 method) julia> for T = Any[Int,Any], op! = Any[push!,pushfirst!], op = Any[length,size], xs = Any[(Int,), (Int,Int,)] let effects = Base.infer_effects(simple_vec_ops, (Type{T},typeof(op!),typeof(op),xs...)) @test Core.Compiler.is_foldable(effects) end end julia> code_typed() do simple_vec_ops(Any, push!, length, Any,nothing,Core.Const(1)) end 1-element Vector{Any}: CodeInfo( 1 ─ return 3 ) => Int64 ```
7ad158f
to
7a6a14c
Compare
# safe version | ||
function getindex(::Type{T}, vals::T...) where T | ||
@inline | ||
@_effect_free_terminates_locally_meta | ||
a = Vector{T}(undef, length(vals)) | ||
@_safeindex for i in 1:length(vals) | ||
a[i] = vals[i] | ||
end | ||
return a | ||
end |
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.
@aviatesk
Is there a reason to add this? It seems unneeded and cause some new ambiguity JuliaArrays/StaticArrays.jl#1153 (comment)
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 added this definition since otherwise we can't const-fold safe getindex
operations due to the :nothrow
effect tainted by the foldr
branch in the unsafe definition.
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.
But it looks like this definition is no longer needed (the vals isa NTuple
branch is const-folded even without constant information propagated)? I will make a PR to delete this.
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.
) Mostly by making use of newly added `:inaccessiblememonly` effect property. Now we can fold simple vector operations like: ```julia julia> function simple_vec_ops(T, op!, op, xs...) a = T[] op!(a, xs...) return op(a) end; simple_vec_ops (generic function with 1 method) julia> for T = Any[Int,Any], op! = Any[push!,pushfirst!], op = Any[length,size], xs = Any[(Int,), (Int,Int,)] let effects = Base.infer_effects(simple_vec_ops, (Type{T},typeof(op!),typeof(op),xs...)) @test Core.Compiler.is_foldable(effects) end end julia> code_typed() do simple_vec_ops(Any, push!, length, Any,nothing,Core.Const(1)) end 1-element Vector{Any}: CodeInfo( 1 ─ return 3 ) => Int64 ```
#47154 mistakenly added `@_safeindex` macro on the `_append!(a::AbstractVector, ::Union{HasLength,HasShape}, iter)` method, although `@_safeindex` is only valid for builtin vectors i.e. `Vector`. This commit adds `isa` check so that `@_safeindex` is only applied to builtin vectors. The `isa` check should be removed at compile time, so it should not affect the runtime performance. closes #49748
#47154 mistakenly added `@_safeindex` macro on the `_append!(a::AbstractVector, ::Union{HasLength,HasShape}, iter)` method, although `@_safeindex` is only valid for builtin vectors i.e. `Vector`. This commit adds `isa` check so that `@_safeindex` is only applied to builtin vectors. The `isa` check should be removed at compile time, so it should not affect the runtime performance. closes #49748
Mostly by making use of newly added
:inaccessiblememonly
effect property.Now we can fold simple vector operations like: