Skip to content

Commit

Permalink
sparam inlining: Handle undefined sparams
Browse files Browse the repository at this point in the history
`_compute_sparams` is not required to be able to resolve an sparam,
it can leave it as a `TypeVar`, in which case accessing it should
throw an UndefVarError. This needs to be appropriately handled in
inlining. We have a test case like this in the test suite, but it
was being papered over by an incorrect check in can_inline_typevar.
Remove that check and implement proper undef checking in inlining.
  • Loading branch information
Keno committed Sep 11, 2022
1 parent 5d9807d commit e730590
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 14 deletions.
59 changes: 45 additions & 14 deletions base/compiler/ssair/inlining.jl
Original file line number Diff line number Diff line change
Expand Up @@ -361,9 +361,10 @@ function ir_inline_item!(compact::IncrementalCompact, idx::Int, argexprs::Vector
if extra_coverage_line != 0
insert_node_here!(compact, NewInstruction(Expr(:code_coverage_effect), Nothing, extra_coverage_line))
end
sp_ssa = nothing
if !validate_sparams(sparam_vals)
# N.B. This works on the caller-side argexprs, (i.e. before the va fixup below)
sparam_vals = insert_node_here!(compact,
sp_ssa = insert_node_here!(compact,
effect_free(NewInstruction(Expr(:call, Core._compute_sparams, def, argexprs...), SimpleVector, topline)))
end
if def.isva
Expand Down Expand Up @@ -398,7 +399,7 @@ function ir_inline_item!(compact::IncrementalCompact, idx::Int, argexprs::Vector
# face of rename_arguments! mutating in place - should figure out
# something better eventually.
inline_compact[idx′] = nothing
stmt′ = ssa_substitute!(idx′, stmt′, argexprs, sig, sparam_vals, linetable_offset, boundscheck, inline_compact)
stmt′ = ssa_substitute!(idx′, stmt′, argexprs, sig, sparam_vals, sp_ssa, linetable_offset, boundscheck, inline_compact)
if isa(stmt′, ReturnNode)
val = stmt′.val
return_value = SSAValue(idx′)
Expand All @@ -425,7 +426,7 @@ function ir_inline_item!(compact::IncrementalCompact, idx::Int, argexprs::Vector
inline_compact = IncrementalCompact(compact, spec.ir, compact.result_idx)
for ((_, idx′), stmt′) in inline_compact
inline_compact[idx′] = nothing
stmt′ = ssa_substitute!(idx′, stmt′, argexprs, sig, sparam_vals, linetable_offset, boundscheck, inline_compact)
stmt′ = ssa_substitute!(idx′, stmt′, argexprs, sig, sparam_vals, sp_ssa, linetable_offset, boundscheck, inline_compact)
if isa(stmt′, ReturnNode)
if isdefined(stmt′, :val)
val = stmt′.val
Expand Down Expand Up @@ -903,7 +904,6 @@ end

function can_inline_typevars(method::Method, argtypes::Vector{Any})
may_have_fcalls(method) && return false
any(@nospecialize(x) -> x isa UnionAll, argtypes[2:end]) && return false
return true
end
can_inline_typevars(m::MethodMatch, argtypes::Vector{Any}) = can_inline_typevars(m.method, argtypes)
Expand Down Expand Up @@ -1724,15 +1724,30 @@ function late_inline_special_case!(
end

function ssa_substitute!(idx::Int, @nospecialize(val), arg_replacements::Vector{Any},
@nospecialize(spsig), spvals::Union{SimpleVector, SSAValue},
@nospecialize(spsig), spvals::SimpleVector,
spvals_ssa::Union{Nothing, SSAValue},
linetable_offset::Int32, boundscheck::Symbol, compact::IncrementalCompact)
compact.result[idx][:flag] &= ~IR_FLAG_INBOUNDS
compact.result[idx][:line] += linetable_offset
return ssa_substitute_op!(val, arg_replacements, spsig, spvals, boundscheck, compact, idx)
return ssa_substitute_op!(val, arg_replacements, spsig, spvals, spvals_ssa, boundscheck, compact, idx)
end

function insert_spval!(compact::IncrementalCompact, idx::Int, spvals_ssa::SSAValue, spidx::Int, do_isdefined::Bool)
ret = insert_node!(compact, SSAValue(idx),
effect_free(NewInstruction(Expr(:call, Core._svec_ref, false, spvals_ssa, spidx), Any)))
tcheck_not = nothing
if do_isdefined
tcheck = insert_node!(compact, SSAValue(idx),
effect_free(NewInstruction(Expr(:call, Core.isa, ret, Core.TypeVar), Bool)))
tcheck_not = insert_node!(compact, SSAValue(idx),
effect_free(NewInstruction(Expr(:call, not_int, tcheck), Bool)))
end
return (ret, tcheck_not)
end

function ssa_substitute_op!(@nospecialize(val), arg_replacements::Vector{Any},
@nospecialize(spsig), spvals::Union{SimpleVector, SSAValue},
@nospecialize(spsig), spvals::SimpleVector,
spvals_ssa::Union{Nothing, SSAValue},
boundscheck::Symbol, compact::IncrementalCompact, idx::Int)
if isa(val, Argument)
return arg_replacements[val.n]
Expand All @@ -1741,20 +1756,36 @@ function ssa_substitute_op!(@nospecialize(val), arg_replacements::Vector{Any},
e = val::Expr
head = e.head
if head === :static_parameter
if isa(spvals, SimpleVector)
return quoted(spvals[e.args[1]::Int])
spidx = e.args[1]::Int
val = spvals[spidx]
if !isa(val, TypeVar) && val !== Vararg
return quoted(val)
else
ret = insert_node!(compact, SSAValue(idx),
effect_free(NewInstruction(Expr(:call, Core._svec_ref, false, spvals, e.args[1]), Any)))
flag = compact[SSAValue(idx)][:flag]
maybe_undef = (flag & IR_FLAG_NOTHROW) == 0 && isa(val, TypeVar)
(ret, tcheck_not) = insert_spval!(compact, idx, spvals_ssa, spidx, maybe_undef)
if maybe_undef
insert_node!(compact, SSAValue(idx),
non_effect_free(NewInstruction(Expr(:throw_undef_if_not, val.name, tcheck_not), Nothing)))
end
return ret
end
elseif head === :cfunction && isa(spvals, SimpleVector)
elseif head === :isdefined && isa(e.args[1], Expr) && e.args[1].head === :static_parameter
spidx = (e.args[1]::Expr).args[1]::Int
val = spvals[spidx]
if !isa(val, TypeVar)
return true
else
(_, tcheck_not) = insert_spval!(compact, idx, spvals_ssa, spidx, true)
return tcheck_not
end
elseif head === :cfunction && spvals_ssa === nothing
@assert !isa(spsig, UnionAll) || !isempty(spvals)
e.args[3] = ccall(:jl_instantiate_type_in_env, Any, (Any, Any, Ptr{Any}), e.args[3], spsig, spvals)
e.args[4] = svec(Any[
ccall(:jl_instantiate_type_in_env, Any, (Any, Any, Ptr{Any}), argt, spsig, spvals)
for argt in e.args[4]::SimpleVector ]...)
elseif head === :foreigncall && isa(spvals, SimpleVector)
elseif head === :foreigncall && spvals_ssa === nothing
@assert !isa(spsig, UnionAll) || !isempty(spvals)
for i = 1:length(e.args)
if i == 2
Expand All @@ -1778,7 +1809,7 @@ function ssa_substitute_op!(@nospecialize(val), arg_replacements::Vector{Any},
isa(val, Union{SSAValue, NewSSAValue}) && return val # avoid infinite loop
urs = userefs(val)
for op in urs
op[] = ssa_substitute_op!(op[], arg_replacements, spsig, spvals, boundscheck, compact, idx)
op[] = ssa_substitute_op!(op[], arg_replacements, spsig, spvals, spvals_ssa, boundscheck, compact, idx)
end
return urs[]
end
9 changes: 9 additions & 0 deletions test/compiler/inline.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1512,3 +1512,12 @@ end
# Test getfield modeling of Type{Ref{_A}} where _A
@test Core.Compiler.getfield_tfunc(Type, Core.Compiler.Const(:parameters)) !== Union{}
@test fully_eliminated(Base.ismutable, Tuple{Base.RefValue})

# TODO: Remove compute sparams for vararg_retrival
fvarargN_inline(x::Tuple{Vararg{Int, N}}) where {N} = N
fvarargN_inline(args...) = fvarargN_inline(args)
let src = code_typed1(fvarargN_inline, (Tuple{Vararg{Int}},))
@test_broken count(iscall((src, Core._compute_sparams)), src.code) == 0 &&
count(iscall((src, Core._svec_ref)), src.code) == 0 &&
count(iscall((src, Core.nfields)), src.code) == 1
end
6 changes: 6 additions & 0 deletions test/core.jl
Original file line number Diff line number Diff line change
Expand Up @@ -7831,3 +7831,9 @@ end
# Correct isdefined error for isdefined of Module of Int fld
f_isdefined_one(@nospecialize(x)) = isdefined(x, 1)
@test (try; f_isdefined_one(@__MODULE__); catch err; err; end).got === 1

# Unspecialized retrieval of vararg length
fvarargN(x::Tuple{Vararg{Int, N}}) where {N} = N
fvarargN(args...) = fvarargN(args)
finvokevarargN() = Base.inferencebarrier(fvarargN)(1, 2, 3)
@test finvokevarargN() == 3

0 comments on commit e730590

Please sign in to comment.