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.

Sign up for GitHub

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

Jump to bottom

follow up the inlining unmatched type param PR #46484

Merged
merged 1 commit into from
Aug 26, 2022
Merged

follow up the inlining unmatched type param PR #46484

merged 1 commit into from
Aug 26, 2022

Conversation

aviatesk
Copy link
Member

This commit follows up #45062:

  • eliminate closure capturing, and improve type stability a bit
  • refactor the test structure so that they are more aligned with
    the other parts of tests

@aviatesk aviatesk requested a review from ianatol August 26, 2022 01:08
@ianatol
Copy link
Member

ianatol commented Aug 26, 2022

Thanks Shuhei!

Another thing that I was working on but wanted to move along the PR was extending compute_inlining_cases to be able to do this same kind of inlining of unmatched type params when dealing with ConstPropResult. This would also maybe allow us to just have one compute_inlining_cases function and re-use some of that code.

The only part I wasn't sure about was trying to do an only_method-like logic but considering info.results where info isa ConstCallInfo. If we can solve that, passing an allow_typevars to resolve_todo is trivial, and we can extend this functionality for const calls. I have it on my todo list for after Cedar stuff, just wanted to write it down somewhere.

@aviatesk
follow up the inlining unmatched type param PR
b26a44e 
This commit follows up #45062:
- eliminate closure capturing, and improve type stability a bit
- refactor the test structure so that they are more aligned with
  the other parts of tests
ianatol
ianatol reviewed Aug 26, 2022
View reviewed changes
base/compiler/ssair/passes.jl
i === nothing && return nothing
_any(j->has_typevar(sig.parameters[j], tvar), i+1:length(sig.parameters)) && return nothing
let sig=sig
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain this? I guess it's related to closure capturing?

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, sometimes local variables used within closures cause capturing, which results in a case the local variable is instantiated as Core.Box. In that case we can use this let magic to prevent the capturing and get more type stability (see https://docs.julialang.org/en/v1/manual/performance-tips/#man-performance-captured this performance tips section for more details).

I caught this capturing with JET as follows:

julia> using JET

# the tweaked version
julia> @eval Core.Compiler @inline function _lift_svec_ref1(def::Expr, compact::IncrementalCompact)
           m = argextype(def.args[2], compact)
           isa(m, Const) || return nothing
           m = m.val
           isa(m, Method) || return nothing
           # TODO: More general structural analysis of the intersection
           length(def.args) >= 3 || return nothing
           sig = m.sig
           isa(sig, UnionAll) || return nothing
           tvar = sig.var
           sig = sig.body
           isa(sig, DataType) || return nothing
           sig.name === Tuple.name || return nothing
           length(sig.parameters) >= 1 || return nothing
       
           i = let sig=sig
               findfirst(j->has_typevar(sig.parameters[j], tvar), 1:length(sig.parameters))
           end
           i === nothing && return nothing
           let sig=sig
               any(j->has_typevar(sig.parameters[j], tvar), i+1:length(sig.parameters))
           end && return nothing
       
           arg = sig.parameters[i]
           isa(arg, DataType) || return nothing
       
           rarg = def.args[2 + i]
           isa(rarg, SSAValue) || return nothing
           argdef = compact[rarg][:inst]
           if isexpr(argdef, :new)
               rarg = argdef.args[1]
               isa(rarg, SSAValue) || return nothing
               argdef = compact[rarg][:inst]
           end
       
           is_known_call(argdef, Core.apply_type, compact) || return nothing
           length(argdef.args) == 3 || return nothing
       
           applyT = argextype(argdef.args[2], compact)
           isa(applyT, Const) || return nothing
           applyT = applyT.val
       
           isa(applyT, UnionAll) || return nothing
           applyTvar = applyT.var
           applyTbody = applyT.body
       
           isa(applyTbody, DataType) || return nothing
           applyTbody.name == arg.name || return nothing
           length(applyTbody.parameters) == length(arg.parameters) == 1 || return nothing
           applyTbody.parameters[1] === applyTvar || return nothing
           arg.parameters[1] === tvar || return nothing
           return LiftedValue(argdef.args[3])
       end
_lift_svec_ref1 (generic function with 1 method)

julia> report_opt(Core.Compiler._lift_svec_ref1, (Expr,Core.Compiler.IncrementalCompact))
No errors detected

# the original version
julia> @eval Core.Compiler @inline function _lift_svec_ref2(def::Expr, compact::IncrementalCompact)
           m = argextype(def.args[2], compact)
           isa(m, Const) || return nothing
           m = m.val
           isa(m, Method) || return nothing
           # TODO: More general structural analysis of the intersection
           length(def.args) >= 3 || return nothing
           sig = m.sig
           isa(sig, UnionAll) || return nothing
           tvar = sig.var
           sig = sig.body
           isa(sig, DataType) || return nothing
           sig.name === Tuple.name || return nothing
           length(sig.parameters) >= 1 || return nothing
       
           i = findfirst(j->has_typevar(sig.parameters[j], tvar), 1:length(sig.parameters))
           i === nothing && return nothing
           any(j->has_typevar(sig.parameters[j], tvar), i+1:length(sig.parameters)) && return nothing
       
           arg = sig.parameters[i]
           isa(arg, DataType) || return nothing
       
           rarg = def.args[2 + i]
           isa(rarg, SSAValue) || return nothing
           argdef = compact[rarg][:inst]
           if isexpr(argdef, :new)
               rarg = argdef.args[1]
               isa(rarg, SSAValue) || return nothing
               argdef = compact[rarg][:inst]
           end
       
           is_known_call(argdef, Core.apply_type, compact) || return nothing
           length(argdef.args) == 3 || return nothing
       
           applyT = argextype(argdef.args[2], compact)
           isa(applyT, Const) || return nothing
           applyT = applyT.val
       
           isa(applyT, UnionAll) || return nothing
           applyTvar = applyT.var
           applyTbody = applyT.body
       
           isa(applyTbody, DataType) || return nothing
           applyTbody.name == arg.name || return nothing
           length(applyTbody.parameters) == length(arg.parameters) == 1 || return nothing
           applyTbody.parameters[1] === applyTvar || return nothing
           arg.parameters[1] === tvar || return nothing
           return LiftedValue(argdef.args[3])
       end
_lift_svec_ref2 (generic function with 1 method)

julia> report_opt(Core.Compiler._lift_svec_ref2, (Expr,Core.Compiler.IncrementalCompact))
═════ 18 possible errors found ═════
┌ @ REPL[71]:16 1 Core.Compiler.:(:) Core.Compiler.length(getfield(sig, :contents).parameters)
│┌ @ range.jl:3 Core.Compiler.promote(a, b)
││┌ @ promotion.jl:381 Core.Compiler._promote(x, y)
│││┌ @ promotion.jl:357 R = Core.Compiler.promote_type(T, S)
││││┌ @ promotion.jl:307 Core.Compiler.promote_result(T, S, Core.Compiler.promote_rule(T, S), Core.Compiler.promote_rule(S, T))
│││││┌ @ promotion.jl:321 Core.Compiler.promote_type(T, S)
││││││┌ @ promotion.jl:307 Core.Compiler.promote_result(T, S, Core.Compiler.promote_rule(T, S), Core.Compiler.promote_rule(S, T))
│││││││┌ @ promotion.jl:324 Core.Compiler.typejoin(T, S)
││││││││┌ @ promotion.jl:36 Core.Compiler.typejoin(a, b.body)
│││││││││┌ @ promotion.jl:40 Core.Compiler.typejoin(b.a, b.b)
││││││││││┌ @ promotion.jl:126 Core.Compiler.UnionAll(%430, %432)
│││││││││││ runtime dispatch detected: Core.Compiler.UnionAll(%430::Any, %432::Any)::Any
││││││││││└────────────────────
│││││││││┌ @ promotion.jl:126 Core.Compiler.UnionAll(%408, %410)
││││││││││ runtime dispatch detected: Core.Compiler.UnionAll(%408::Any, %410::Any)::Any
│││││││││└────────────────────
││││││││┌ @ promotion.jl:126 Core.Compiler.UnionAll(%403, %405)
│││││││││ runtime dispatch detected: Core.Compiler.UnionAll(%403::Any, %405::Any)::Any
││││││││└────────────────────
││┌ @ promotion.jl:382 Core.Compiler.not_sametype(tuple(x, y), tuple(px, py))
│││┌ @ promotion.jl:399 Core.Compiler.sametype_error(x)
││││┌ @ promotion.jl:405 Core.Compiler.map(#43, input)
│││││┌ @ tuple.jl:274 f(t[1])
││││││┌ @ promotion.jl:406 %1()
│││││││ runtime dispatch detected: %1::Any(::Int64)::Any
││││││└────────────────────
││││┌ @ promotion.jl:405 Core.Compiler.error("promotion of types ", Core.Compiler.join(Core.Compiler.map(#43, input), ", ", " and "), " failed to change any arguments")
│││││┌ @ error.jl:44 Base.string(s...)
││││││┌ @ strings/io.jl:185 Base.print_to_string(xs...)
│││││││┌ @ strings/io.jl:144 print(%79, %82)
││││││││ runtime dispatch detected: print(%79::IOBuffer, %82::Any)::Any
│││││││└─────────────────────
┌ @ REPL[71]:2 sig = Core.Box()
│ captured variable `sig` detected
└──────────────
┌ @ REPL[71]:14 Core.Compiler.length(%69)
│ runtime dispatch detected: Core.Compiler.length(%69::Any)::Any
└───────────────
┌ @ REPL[71]:14 %70 Core.Compiler.:>= 1
│ runtime dispatch detected: (%70::Any Core.Compiler.:>= 1)::Any
└───────────────
┌ @ REPL[71]:16 Core.Compiler.length(%83)
│ runtime dispatch detected: Core.Compiler.length(%83::Any)::Any
└───────────────
┌ @ REPL[71]:16 1 Core.Compiler.:(:) %84
│ runtime dispatch detected: (1 Core.Compiler.:(:) %84::Any)::Any
└───────────────
┌ @ REPL[71]:16 Core.Compiler.findfirst(%77, %99)
│ runtime dispatch detected: Core.Compiler.findfirst(%77::Core.Compiler.var"#502#504", %99::Any)::Any
└───────────────
┌ @ REPL[71]:18 %100 Core.Compiler.:+ 1
│ runtime dispatch detected: (%100::Any Core.Compiler.:+ 1)::Any
└───────────────
┌ @ REPL[71]:18 Core.Compiler.length(%114)
│ runtime dispatch detected: Core.Compiler.length(%114::Any)::Any
└───────────────
┌ @ REPL[71]:18 %108 Core.Compiler.:(:) %115
│ runtime dispatch detected: (%108::Any Core.Compiler.:(:) %115::Any)::Any
└───────────────
┌ @ REPL[71]:18 Core.Compiler.any(%107, %116)
│ runtime dispatch detected: Core.Compiler.any(%107::Core.Compiler.var"#503#505", %116::Any)::Bool
└───────────────
┌ @ REPL[71]:20 %125[%100]
│ runtime dispatch detected: (%125::Any)[%100::Any]::Any
└───────────────
┌ @ REPL[71]:23 2 Core.Compiler.:+ %100
│ runtime dispatch detected: (2 Core.Compiler.:+ %100::Any)::Any
└───────────────
┌ @ REPL[71]:23 %130[%131]
│ runtime dispatch detected: (%130::Vector{Any})[%131::Any]::Any
└───────────────

@aviatesk
Copy link
Member Author

Another thing that I was working on but wanted to move along the PR was extending compute_inlining_cases to be able to do this same kind of inlining of unmatched type params when dealing with ConstPropResult. This would also maybe allow us to just have one compute_inlining_cases function and re-use some of that code.

That sounds to be a good improvement. Let's do it as a separate PR after finishing up the other immediate tasks assigned to us.

@aviatesk aviatesk merged commit bea7b6f into master Aug 26, 2022
@aviatesk aviatesk deleted the avi/follow-45062 branch August 26, 2022 04:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ianatol ianatol ianatol left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aviatesk @ianatol