Skip to content

Commit

Permalink
Fix constprop inference for varargs OpaqueClosure (#39972)
Browse files Browse the repository at this point in the history
OpaqueClosure are marked as vararg (or not) at construction time
rather than in the method. As a result, we need to chain this
information through to the cache. We may want to refactor this
code to deal with the vararg-ness (or not) of a particular
definition one level above this. We already keep the cache
in a form that represents the vararg tuple explicitly.
However, for now this fixes things for now (and also
prevents Cthulhu from getting confused upon encountering
these).
  • Loading branch information
Keno authored Mar 12, 2021
1 parent ec78ac7 commit a43e01f
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 14 deletions.
13 changes: 8 additions & 5 deletions base/compiler/abstractinterpretation.jl
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ function abstract_call_gf_by_type(interp::AbstractInterpreter, @nospecialize(f),
push!(edges, edge)
end
this_argtypes = applicable_argtypes === nothing ? argtypes : applicable_argtypes[i]
const_rt, const_result = abstract_call_method_with_const_args(interp, rt, f, this_argtypes, match, sv, edgecycle)
const_rt, const_result = abstract_call_method_with_const_args(interp, rt, f, this_argtypes, match, sv, edgecycle, false)
if const_rt !== rt && const_rt rt
rt = const_rt
end
Expand All @@ -163,7 +163,7 @@ function abstract_call_gf_by_type(interp::AbstractInterpreter, @nospecialize(f),
# try constant propagation with argtypes for this match
# this is in preparation for inlining, or improving the return result
this_argtypes = applicable_argtypes === nothing ? argtypes : applicable_argtypes[i]
const_this_rt, const_result = abstract_call_method_with_const_args(interp, this_rt, f, this_argtypes, match, sv, edgecycle)
const_this_rt, const_result = abstract_call_method_with_const_args(interp, this_rt, f, this_argtypes, match, sv, edgecycle, false)
if const_this_rt !== this_rt && const_this_rt this_rt
this_rt = const_this_rt
end
Expand Down Expand Up @@ -470,7 +470,8 @@ end

function abstract_call_method_with_const_args(interp::AbstractInterpreter, @nospecialize(rettype),
@nospecialize(f), argtypes::Vector{Any}, match::MethodMatch,
sv::InferenceState, edgecycle::Bool)
sv::InferenceState, edgecycle::Bool,
va_override::Bool)
mi = maybe_get_const_prop_profitable(interp, rettype, f, argtypes, match, sv, edgecycle)
mi === nothing && return Any, nothing
# try constant prop'
Expand All @@ -496,7 +497,7 @@ function abstract_call_method_with_const_args(interp::AbstractInterpreter, @nosp
end
end
end
inf_result = InferenceResult(mi, argtypes)
inf_result = InferenceResult(mi, argtypes, va_override)
frame = InferenceState(inf_result, #=cache=#false, interp)
frame === nothing && return Any, nothing # this is probably a bad generated function (unsound), but just ignore it
frame.parent = sv
Expand Down Expand Up @@ -1236,7 +1237,9 @@ function abstract_call_opaque_closure(interp::AbstractInterpreter, closure::Part
rt, edgecycle, edge = abstract_call_method(interp, closure.source::Method, sig, Core.svec(), false, sv)
info = OpaqueClosureCallInfo(edge)
if !edgecycle
const_rettype, result = abstract_call_method_with_const_args(interp, rt, closure, argtypes, MethodMatch(sig, Core.svec(), closure.source::Method, false), sv, edgecycle)
const_rettype, result = abstract_call_method_with_const_args(interp, rt, closure, argtypes,
MethodMatch(sig, Core.svec(), closure.source::Method, false),
sv, edgecycle, closure.isva)
if const_rettype rt
rt = const_rettype
end
Expand Down
12 changes: 6 additions & 6 deletions base/compiler/inferenceresult.jl
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@ end
# for the provided `linfo` and `given_argtypes`. The purpose of this function is
# to return a valid value for `cache_lookup(linfo, argtypes, cache).argtypes`,
# so that we can construct cache-correct `InferenceResult`s in the first place.
function matching_cache_argtypes(linfo::MethodInstance, given_argtypes::Vector)
function matching_cache_argtypes(linfo::MethodInstance, given_argtypes::Vector, va_override)
@assert isa(linfo.def, Method) # ensure the next line works
nargs::Int = linfo.def.nargs
@assert length(given_argtypes) >= (nargs - 1)
given_argtypes = anymap(widenconditional, given_argtypes)
if linfo.def.isva
if va_override || linfo.def.isva
isva_given_argtypes = Vector{Any}(undef, nargs)
for i = 1:(nargs - 1)
isva_given_argtypes[i] = argtype_by_index(given_argtypes, i)
Expand All @@ -30,7 +30,7 @@ function matching_cache_argtypes(linfo::MethodInstance, given_argtypes::Vector)
end
given_argtypes = isva_given_argtypes
end
cache_argtypes, overridden_by_const = matching_cache_argtypes(linfo, nothing)
cache_argtypes, overridden_by_const = matching_cache_argtypes(linfo, nothing, va_override)
if nargs === length(given_argtypes)
for i in 1:nargs
given_argtype = given_argtypes[i]
Expand Down Expand Up @@ -134,10 +134,10 @@ function most_general_argtypes(method::Union{Method, Nothing}, @nospecialize(spe
cache_argtypes
end

function matching_cache_argtypes(linfo::MethodInstance, ::Nothing)
function matching_cache_argtypes(linfo::MethodInstance, ::Nothing, va_override::Bool)
mthd = isa(linfo.def, Method) ? linfo.def::Method : nothing
cache_argtypes = most_general_argtypes(mthd, linfo.specTypes, isa(mthd, Method) ?
mthd.isva : false)
cache_argtypes = most_general_argtypes(mthd, linfo.specTypes,
va_override || (isa(mthd, Method) ? mthd.isva : false))
return cache_argtypes, falses(length(cache_argtypes))
end

Expand Down
2 changes: 1 addition & 1 deletion base/compiler/ssair/legacy.jl
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
function inflate_ir(ci::CodeInfo, linfo::MethodInstance)
sptypes = sptypes_from_meth_instance(linfo)
if ci.inferred
argtypes, _ = matching_cache_argtypes(linfo, nothing)
argtypes, _ = matching_cache_argtypes(linfo, nothing, false)
else
argtypes = Any[ Any for i = 1:length(ci.slotflags) ]
end
Expand Down
4 changes: 2 additions & 2 deletions base/compiler/types.jl
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ mutable struct InferenceResult
result # ::Type, or InferenceState if WIP
src #::Union{CodeInfo, OptimizationState, Nothing} # if inferred copy is available
valid_worlds::WorldRange # if inference and optimization is finished
function InferenceResult(linfo::MethodInstance, given_argtypes = nothing)
argtypes, overridden_by_const = matching_cache_argtypes(linfo, given_argtypes)
function InferenceResult(linfo::MethodInstance, given_argtypes = nothing, va_override=false)
argtypes, overridden_by_const = matching_cache_argtypes(linfo, given_argtypes, va_override)
return new(linfo, argtypes, overridden_by_const, Any, nothing, WorldRange())
end
end
Expand Down
7 changes: 7 additions & 0 deletions test/opaque_closure.jl
Original file line number Diff line number Diff line change
Expand Up @@ -182,3 +182,10 @@ end
end
@test isa(oc_trivial_generated(), Core.OpaqueClosure{Tuple{}, Any})
@test oc_trivial_generated()() == 1

# Constprop through varargs OpaqueClosure
function oc_varargs_constprop()
oc = @opaque (args...)->args[1]+args[2]+args[3]
return Val{oc(1,2,3)}()
end
Base.return_types(oc_varargs_constprop, Tuple{}) == Any[Val{6}]

4 comments on commit a43e01f

@vtjnash
Copy link
Member

Choose a reason for hiding this comment

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

@nanosoldier runbenchmarks(ALL, isdaily=true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Executing the daily package evaluation, I will reply here when finished:

@nanosoldier runtests(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

Please sign in to comment.