Skip to content

effects: improve idempotency of effects derived by post-opt analysis #52085

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

Merged
merged 3 commits into from
Nov 9, 2023
Merged

effects: improve idempotency of effects derived by post-opt analysis #52085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eafa1e3
fix up test case for unmatched type param inlining
aviatesk Nov 9, 2023
81ef12a
fixed test case for #43296
aviatesk Nov 9, 2023
358540c
effects: improve idempotency of effects derived by post-opt analysis
aviatesk Nov 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions base/compiler/ssair/inlining.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1393,8 +1393,7 @@ function compute_inlining_cases(@nospecialize(info::CallInfo), flag::UInt32, sig
cases = InliningCase[]
argtypes = sig.argtypes
local handled_all_cases::Bool = true
local revisit_idx = nothing
local only_method = nothing
local revisit_idx = local only_method = nothing
local meth::MethodLookupResult
local all_result_count = 0
local joint_effects::Effects = EFFECTS_TOTAL
Expand All @@ -1410,14 +1409,14 @@ function compute_inlining_cases(@nospecialize(info::CallInfo), flag::UInt32, sig
handled_all_cases = false
continue
else
if length(meth) == 1 && only_method !== false
if length(meth) == 1 && only_method !== missing
if only_method === nothing
only_method = meth[1].method
elseif only_method !== meth[1].method
only_method = false
only_method = missing
end
else
only_method = false
only_method = missing
end
end
local split_fully_covered::Bool = false
Expand Down
4 changes: 2 additions & 2 deletions base/compiler/typeinfer.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ function finish(me::InferenceState, interp::AbstractInterpreter)
end
me.result.valid_worlds = me.valid_worlds
me.result.result = bestguess
me.ipo_effects = me.result.ipo_effects = adjust_effects(me)
me.result.ipo_effects = me.ipo_effects = adjust_effects(me)

if limited_ret
# a parent may be cached still, but not this intermediate work:
Expand Down Expand Up @@ -856,7 +856,7 @@ function typeinf_edge(interp::AbstractInterpreter, method::Method, @nospecialize
update_valid_age!(caller, frame.valid_worlds)
isinferred = is_inferred(frame)
edge = isinferred ? mi : nothing
effects = isinferred ? frame.ipo_effects : adjust_effects(Effects(), method) # effects are adjusted already within `finish` for ipo_effects
effects = isinferred ? frame.result.ipo_effects : adjust_effects(Effects(), method) # effects are adjusted already within `finish` for ipo_effects
# propagate newly inferred source to the inliner, allowing efficient inlining w/o deserialization:
# note that this result is cached globally exclusively, we can use this local result destructively
volatile_inf_result = isinferred && let inferred_src = result.src
Expand Down
14 changes: 14 additions & 0 deletions test/compiler/effects.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1158,3 +1158,17 @@ end
issue51837(; openquotechar, newlinechar)
end |> !Core.Compiler.is_nothrow
@test_throws ArgumentError issue51837(; openquotechar='α', newlinechar='\n')

# idempotency of effects derived by post-opt analysis
callgetfield(x, f) = getfield(x, f, Base.@_boundscheck)
@test Base.infer_effects(callgetfield, (Some{Any},Symbol)).noub === Core.Compiler.NOUB_IF_NOINBOUNDS
callgetfield1(x, f) = getfield(x, f, Base.@_boundscheck)
callgetfield_simple(x, f) = callgetfield1(x, f)
@test Base.infer_effects(callgetfield_simple, (Some{Any},Symbol)).noub ===
Base.infer_effects(callgetfield_simple, (Some{Any},Symbol)).noub ===
Core.Compiler.ALWAYS_TRUE
callgetfield2(x, f) = getfield(x, f, Base.@_boundscheck)
callgetfield_inbounds(x, f) = @inbounds callgetfield2(x, f)
@test Base.infer_effects(callgetfield_inbounds, (Some{Any},Symbol)).noub ===
Base.infer_effects(callgetfield_inbounds, (Some{Any},Symbol)).noub ===
Core.Compiler.ALWAYS_FALSE
5 changes: 3 additions & 2 deletions test/compiler/inference.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ end
Core.Compiler.limit_type_size(Type{Any}, Type, Union{}, 0, 0) ==
Type{Any}

# issue #43296 #43296
# issue #43296
struct C43296{t,I} end
r43296(b) = r43296(typeof(b))
r43296(::Type) = nothing
Expand All @@ -149,7 +149,8 @@ f43296(g, :) = h
k43296(b, j, :) = l
k43296(b, j, ::Nothing) = b
i43296(b, j) = k43296(b, j, r43296(j))
@test only(Base.return_types(i43296, (Int, C43296{C43296{C43296{Val, Tuple}, Tuple}}))) == Int
@test only(Base.return_types(i43296, (Int, C43296{C43296{C43296{Val, Tuple}}}))) <: Int
Copy link
Member Author

@aviatesk aviatesk Nov 9, 2023

Choose a reason for hiding this comment

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

This call signature would always result in an error, which can now be discovered by concrete-evaluation.

@test only(Base.return_types(i43296, (Int, C43296{C43296{C43296{Val, <:Tuple}}}))) <: Int

abstract type e43296{a, j} <: AbstractArray{a, j} end
abstract type b43296{a, j, c, d} <: e43296{a, j} end
Expand Down
25 changes: 11 additions & 14 deletions test/compiler/inline.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1675,31 +1675,28 @@ function oc_capture_oc(z)
end
@test fully_eliminated(oc_capture_oc, (Int,))

# inlining with unmatched type parameters
@eval struct OldVal{T}
x::T
Copy link
Member Author

@aviatesk aviatesk Nov 9, 2023

Choose a reason for hiding this comment

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

The previous definition was actually invalid, causing segfault when concretized.

(OV::Type{OldVal{T}})() where T = $(Expr(:new, :OV))
end
with_unmatched_typeparam1(x::OldVal{i}) where {i} = i
with_unmatched_typeparam2() = [ Base.donotdelete(OldVal{i}()) for i in 1:10000 ]
function with_unmatched_typeparam3()
@test OldVal{0}() === OldVal{0}.instance
function with_unmatched_typeparam()
f(x::OldVal{i}) where {i} = i
r = 0
for i = 1:10000
r += f(OldVal{i}())
end
return r
end

@testset "Inlining with unmatched type parameters" begin
let src = code_typed1(with_unmatched_typeparam1, (Any,))
@test !any(@nospecialize(x) -> isexpr(x, :call) && length(x.args) == 1, src.code)
end
let src = code_typed1(with_unmatched_typeparam2)
@test !any(@nospecialize(x) -> isexpr(x, :call) && length(x.args) == 1, src.code)
end
let src = code_typed1(with_unmatched_typeparam3)
@test !any(@nospecialize(x) -> isexpr(x, :call) && length(x.args) == 1, src.code)
let src = code_typed1(with_unmatched_typeparam)
found = nothing
for x in src.code
if isexpr(x, :call) && length(x.args) == 1
found = x
break
end
end
@test isnothing(found) || (source=src, statement=found)
end

function twice_sitofp(x::Int, y::Int)
Expand Down