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

optimizer: propagate callsite inlining annotation across kwfunc #43911

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aviatesk
Copy link
Member

The callsite inlining annotations only works per-frame, but it might
appear unintuitive when applied to kwfuncs. This commits adds an extra
effort to propagate callsite inlining annotations across kwfunc abstraction:

julia> function someeval(@nospecialize(x); mod=Main)
           v = Core.eval(mod, :(x = $(esc(x)))) # by default this prevents inlining
           return v
       end
someeval (generic function with 1 method)

julia> let src = code_typed1((Any,)) do x
               someeval(x; mod=Main)
           end
           @test count(iscall((src, Core._expr)), src.code) == 0

           src = code_typed1((Any,)) do x
               @inline someeval(x; mod=Main)
           end
           @test count(iscall((src, Core._expr)), src.code) == 2
       end
Test Passed

@aviatesk aviatesk requested a review from vtjnash January 24, 2022 16:25
@aviatesk aviatesk added the compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) label Jan 24, 2022
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

This could be somewhat highly aggressive, since the user can include arbitrary code in the kwsorter (via default arguments), but seems fairly reasonable anyways

@@ -822,6 +822,8 @@ function typeinf_edge(interp::AbstractInterpreter, method::Method, @nospecialize
unlock_mi_inference(interp, mi)
return Any, nothing
end
caller_kwfunc_inlining = caller.kwfunc_inlining
caller_kwfunc_inlining !== nothing && propagate_caller_annotations!(caller_kwfunc_inlining, frame)
Copy link
Member

Choose a reason for hiding this comment

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

hm, wait, can't there be any number of callers, and this is just the first one that we saw, before we put it in the cache?

sv.kwfunc_inlining = true
elseif is_stmt_noinline(flag)
sv.kwfunc_inlining = false
end
Copy link
Member

Choose a reason for hiding this comment

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

this seems like some very incorrect global state changes for the containing method?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, somehow I assumed the containing method only has a single call of kwfunc at most, but it isn't true really.
The right way to support that is probably to propagate this information with a dedicated a lattice element.

@vtjnash
Copy link
Member

vtjnash commented Jan 24, 2022

This also comes up with positional arguments. For example:

f(a, b = (for _ in 1:10; for _ in 1:20; for _ in 1:30; end; end; end)) = a
g() = f(0)

So I am not sure anymore that we should do this.

The callsite inlining annotations only works per-frame, but it might
appear unintuitive when applied to kwfuncs. This commits adds an extra
effort to propagate callsite inlining annotations across kwfunc abstraction:
```julia
julia> function someeval(@nospecialize(x); mod=Main)
           v = Core.eval(mod, :(x = $(esc(x)))) # by default this prevents inlining
           return v
       end
someeval (generic function with 1 method)

julia> let src = code_typed1((Any,)) do x
               someeval(x; mod=Main)
           end
           @test count(iscall((src, Core._expr)), src.code) == 0

           src = code_typed1((Any,)) do x
               @inline someeval(x; mod=Main)
           end
           @test count(iscall((src, Core._expr)), src.code) == 2
       end
Test Passed
```
@aviatesk
Copy link
Member Author

This also comes up with positional arguments. For example:

f(a, b = (for _ in 1:10; for _ in 1:20; for _ in 1:30; end; end; end)) = a
g() = f(0)

So I am not sure anymore that we should do this.

yes, but for positional arguments we can enforce inlining by listing up all arguments (very verbose though), but for keyword arguments we currently don't have a way to enforce inlining.

@aviatesk
Copy link
Member Author

Now I setup a new lattice element Kwfunc so that callsite inlining information on kwfunc call is propagated locally.
Though now this PR comes with some complexity, and I'm not too sure we really want to support this niche.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:optimizer Optimization passes (mostly in base/compiler/ssair/)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants