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

propagate method metadata to keyword sorter methods #45041

Merged
merged 3 commits into from
Jul 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions base/compiler/abstractinterpretation.jl
Original file line number Diff line number Diff line change
Expand Up @@ -799,8 +799,7 @@ function const_prop_enabled(interp::AbstractInterpreter, sv::InferenceState, mat
add_remark!(interp, sv, "[constprop] Disabled by parameter")
return false
end
method = match.method
if method.constprop == 0x02
if is_no_constprop(match.method)
add_remark!(interp, sv, "[constprop] Disabled by method parameter")
return false
end
Expand Down Expand Up @@ -1003,7 +1002,7 @@ function is_all_overridden((; fargs, argtypes)::ArgInfo, sv::InferenceState)
end

function force_const_prop(interp::AbstractInterpreter, @nospecialize(f), method::Method)
return method.constprop == 0x01 ||
return is_aggressive_constprop(method) ||
InferenceParams(interp).aggressive_constant_propagation ||
istopfunction(f, :getproperty) ||
istopfunction(f, :setproperty!)
Expand Down
14 changes: 14 additions & 0 deletions base/compiler/utilities.jl
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,20 @@ function specialize_method(match::MethodMatch; kwargs...)
return specialize_method(match.method, match.spec_types, match.sparams; kwargs...)
end

"""
is_aggressive_constprop(method::Union{Method,CodeInfo}) -> Bool

Check if `method` is declared as `Base.@constprop :aggressive`.
"""
is_aggressive_constprop(method::Union{Method,CodeInfo}) = method.constprop == 0x01

"""
is_no_constprop(method::Union{Method,CodeInfo}) -> Bool

Check if `method` is declared as `Base.@constprop :none`.
"""
is_no_constprop(method::Union{Method,CodeInfo}) = method.constprop == 0x02

#########
# types #
#########
Expand Down
15 changes: 15 additions & 0 deletions src/ast.scm
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,21 @@
(and (if one (length= e 3) (length> e 2))
(eq? (car e) 'meta) (memq (cadr e) '(nospecialize specialize))))

(define (meta? e)
(and (length> e 1) (eq? (car e) 'meta)))

(define (method-meta-sym? x)
(memq x '(inline noinline aggressive_constprop no_constprop propagate_inbounds)))

(define (propagate-method-meta e)
`(meta ,@(filter (lambda (x)
(or (method-meta-sym? x)
(and (pair? x) (eq? (car x) 'purity))))
Copy link
Member Author

@JeffBezanson JeffBezanson Jul 6, 2022

Choose a reason for hiding this comment

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

For purity we can only keep the ones that are true of keyword sorters. Safer not to propagate them at all. But I think the following apply to keyword sorters:

  • consistent
  • effect_free
  • terminates_globally
  • notaskstate

But please check me on that!

Copy link
Member

Choose a reason for hiding this comment

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

I think keyword sorters are usually:

  • :consistent
  • :effect_free
  • :terminates_locally (:terminates_globally requires that of the body function itself)
  • :notaskstate

but they are not :nothrow (since they can throw any of TypeError/UndefKeywordError/MethodError).

Well, I'd argue it is okay to propagate the effects assumed by Base.@assume_effects to keyword sorters, since the point of using the macro to override the conclusion that is otherwise derived by the effect analysis.
If we write

Base.@assume_effects :total specific(A::Type, B::Type; v::Number=1) =
   v * one(ccall(:jl_type_morespecific, Cint, (Any, Any), A, B)  0 ? A : B)

then IMO we'd like to assume both specific(Int, Integer) and specific(Int, Integer; v=42) are :total (and therefore :nothrow also)?

(cdr e))))

(define (argwide-nospecialize-meta? e)
(and (length= e 2) (eq? (car e) 'meta) (memq (cadr e) '(nospecialize specialize))))

(define (if-generated? e)
(and (length= e 4) (eq? (car e) 'if) (equal? (cadr e) '(generated))))

Expand Down
3 changes: 3 additions & 0 deletions src/julia-syntax.scm
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,9 @@
,(if (any kwarg? pargl) (gensy) UNUSED)
(call (core kwftype) ,ftype)) ,kw ,@pargl ,@vararg)
`(block
;; propagate method metadata to keyword sorter
,@(map propagate-method-meta (filter meta? prologue))
Copy link
Member Author

Choose a reason for hiding this comment

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

Should filter out expressions that end up empty, (meta).

Copy link
Member

Choose a reason for hiding this comment

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

(filter meta? prologue) should already filter out the empty (meta) case?

(and (length> e 1) (eq? (car e) 'meta)))

,@(filter argwide-nospecialize-meta? prologue)
,@(let ((lnns (filter linenum? prologue)))
(if (pair? lnns)
(list (car lnns))
Expand Down
46 changes: 46 additions & 0 deletions test/compiler/inline.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1402,3 +1402,49 @@ end
end
end
end

# https://github.com/JuliaLang/julia/issues/45050
@testset "propagate :meta annotations to keyword sorter methods" begin
# @inline, @noinline, @constprop
let @inline f(::Any; x::Int=1) = 2x
@test ccall(:jl_ir_flag_inlineable, Bool, (Any,), only(methods(f)).source)
@test ccall(:jl_ir_flag_inlineable, Bool, (Any,), only(methods(Core.kwfunc(f))).source)
end
let @noinline f(::Any; x::Int=1) = 2x
@test !ccall(:jl_ir_flag_inlineable, Bool, (Any,), only(methods(f)).source)
@test !ccall(:jl_ir_flag_inlineable, Bool, (Any,), only(methods(Core.kwfunc(f))).source)
end
let Base.@constprop :aggressive f(::Any; x::Int=1) = 2x
@test Core.Compiler.is_aggressive_constprop(only(methods(f)))
@test Core.Compiler.is_aggressive_constprop(only(methods(Core.kwfunc(f))))
end
let Base.@constprop :none f(::Any; x::Int=1) = 2x
@test Core.Compiler.is_no_constprop(only(methods(f)))
@test Core.Compiler.is_no_constprop(only(methods(Core.kwfunc(f))))
end
# @nospecialize
let f(@nospecialize(A::Any); x::Int=1) = 2x
@test only(methods(f)).nospecialize == 1
@test only(methods(Core.kwfunc(f))).nospecialize == 4
end
let f(::Any; x::Int=1) = (@nospecialize; 2x)
@test only(methods(f)).nospecialize == -1
@test only(methods(Core.kwfunc(f))).nospecialize == -1
end
# Base.@assume_effects
let Base.@assume_effects :notaskstate f(::Any; x::Int=1) = 2x
@test Core.Compiler.decode_effects_override(only(methods(f)).purity).notaskstate
@test Core.Compiler.decode_effects_override(only(methods(Core.kwfunc(f))).purity).notaskstate
end
# propagate multiple metadata also
let @inline Base.@assume_effects :notaskstate Base.@constprop :aggressive f(::Any; x::Int=1) = (@nospecialize; 2x)
@test ccall(:jl_ir_flag_inlineable, Bool, (Any,), only(methods(f)).source)
@test Core.Compiler.is_aggressive_constprop(only(methods(f)))
@test ccall(:jl_ir_flag_inlineable, Bool, (Any,), only(methods(Core.kwfunc(f))).source)
@test Core.Compiler.is_aggressive_constprop(only(methods(Core.kwfunc(f))))
@test only(methods(f)).nospecialize == -1
@test only(methods(Core.kwfunc(f))).nospecialize == -1
@test Core.Compiler.decode_effects_override(only(methods(f)).purity).notaskstate
@test Core.Compiler.decode_effects_override(only(methods(Core.kwfunc(f))).purity).notaskstate
end
end