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

Conversation

JeffBezanson
Copy link
Sponsor Member

Arguably, if a method is declared @inline we should propagate that to all the methods we generate for it, including the keyword sorter. However, AFAICT #45028 still needs a @constprop declaration to function.

@aviatesk aviatesk self-requested a review April 20, 2022 22:56
@aviatesk
Copy link
Sponsor Member

Do we want to generalize this to other :meta annotations?

  • :aggressive_constprop/:no_constprop
  • :propagate_inbounds
  • maybe :pure and :purity also?
julia> lwr = Meta.lower(@__MODULE__, quote
           Base.@constprop :aggressive function foo(pos; kw)
               pos + kw
           end
       end);

julia> (lwr.args[1].code[16].args[3]::Core.CodeInfo).constprop
0x01

julia> (lwr.args[1].code[24].args[3]::Core.CodeInfo).constprop # kwsorter
0x00

AFAICT #45028 still needs a @constprop declaration to function.

Yes, I also found that @inline _cat(catdims, A::AbstractArray{T}...) where {T} = cat_t(T, A...; dims=catdims) isn't enough to enable constprop' since the body of cat_t without constant information is still too complex and it won't be jl_ir_flag_inlineable even with the extra inlineability cost (it contains 4 dynamic dispatches).

@timholy
Copy link
Sponsor Member

timholy commented Apr 26, 2022

Do we want to generalize this to other :meta annotations?

Yes please (see #45050). The inability to block constprop on certain "derived" methods is a substantial contribution to latency in packages that have otherwise been carefully TTFX-optimized. I've even added workarounds in some cases, e.g., https://github.com/JuliaImages/ImageView.jl/blob/786fbf584b39387d2d3b5964e4cb40fcb4a3daf3/src/ImageView.jl#L743-L751

@aviatesk aviatesk force-pushed the jb/inlinekwsort branch 2 times, most recently from 0635759 to e4fe26d Compare July 5, 2022 06:28
Copy link
Sponsor Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

I rebased this PR and also added more supports for propagating other metadata to keyword sorter methods too.
I also added test cases and now all the cases reported in #45050 should be covered by this PR.

Although I'm sure this PR is working, I'd like to have a review from others especially on flisp code, since I'm not very familiar with the codebase.

@aviatesk
Copy link
Sponsor Member

aviatesk commented Jul 5, 2022

@nanosoldier runbenchmarks("inference", vs=":master")

src/julia-syntax.scm Outdated Show resolved Hide resolved
@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

src/ast.scm Outdated Show resolved Hide resolved
@aviatesk aviatesk force-pushed the jb/inlinekwsort branch 2 times, most recently from 2c8ce32 to a4e275b Compare July 6, 2022 15:55
@aviatesk aviatesk changed the title propagate inlining metadata to keyword sorter methods propagate method metadata to keyword sorter methods Jul 6, 2022
@@ -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
Sponsor 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
Sponsor 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)))

(define (propagate-method-meta e)
`(meta ,@(filter (lambda (x)
(or (method-meta-sym? x)
(and (pair? x) (eq? (car x) 'purity))))
Copy link
Sponsor 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
Sponsor 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)?

@aviatesk
Copy link
Sponsor Member

@JeffBezanson Can I request your review on this again? I'd like to move this forward once CI runs green.

@aviatesk aviatesk merged commit 9edac22 into master Jul 13, 2022
@aviatesk aviatesk deleted the jb/inlinekwsort branch July 13, 2022 03:30
ffucci pushed a commit to ffucci/julia that referenced this pull request Aug 11, 2022
pcjentsch pushed a commit to pcjentsch/julia that referenced this pull request Aug 18, 2022
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.

None yet

4 participants