Skip to content

Commit

Permalink
inference: follow up #54323, override ssaflags with new cycle effects
Browse files Browse the repository at this point in the history
#54323 ensures that all frames within a cycle have the
same, cycle valid effects. However, `src.ssaflags` is calculated using
partial effects, so when the effects of a `frame` within the cycle are
updated, there would be an inconsistency between `frame.ipo_effects` and
`frame.src.ssaflags`. Due to this inconsistency, #54323
breaks the test cases from #51092, when backported to
v1.11. On the surface this is because #52999 hasn't been
backported to v1.11, but the fundamental issue lies in this
inconsistency between cycle effects and `ssaflags`.

This commit uses a somewhat hacky approach to resolve this. It
identifies statements involved in the cycle comparing `stmt_edges` to
`callers_in_cycle`, and updates `ssaflags` according to new cycle valid
effects if necessary. This resolves the issue, but ideally, it should be
implemented more safely with the new `edges` format that will be
implemented in the future. For now, this approach should be okay.
  • Loading branch information
aviatesk committed Jun 5, 2024
1 parent 06a90c5 commit 878ca04
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 1 deletion.
38 changes: 37 additions & 1 deletion base/compiler/typeinfer.jl
Original file line number Diff line number Diff line change
Expand Up @@ -231,11 +231,46 @@ function finish!(interp::AbstractInterpreter, caller::InferenceState)
return nothing
end

# override statement flags for a frame within the cycle so that they are consistent with
# the new effects that are valid for the cycle
function cycle_ssaflags!(sv::InferenceState, callers_in_cycle::Vector{InferenceState},
cycle_effects::Effects)
new_flags = flags_for_effects(cycle_effects)
old_currpc = sv.currpc
for i = 1:length(sv.src.ssaflags)
edges = sv.stmt_edges[i]
edges === nothing && continue
if any(callers_in_cycle) do caller::InferenceState
mi = caller.linfo
for edge in edges
edge isa MethodInstance || continue
if edge === mi
return true
end
end
return false
end
sv.currpc = i
set_curr_ssaflag!(sv, new_flags)
end
end
sv.currpc = old_currpc
return nothing
end

function _typeinf(interp::AbstractInterpreter, frame::InferenceState)
typeinf_nocycle(interp, frame) || return false # frame is now part of a higher cycle
# with no active ip's, frame is done
frames = frame.callers_in_cycle
isempty(frames) && push!(frames, frame)
if isempty(frames)
push!(frames, frame)
has_cycle = false
elseif length(frames) == 1
@assert frames[1] === frame "invalid callers_in_cycle"
has_cycle = false
else
has_cycle = true
end
cycle_valid_worlds = WorldRange()
cycle_effects = EFFECTS_TOTAL
for caller in frames
Expand All @@ -251,6 +286,7 @@ function _typeinf(interp::AbstractInterpreter, frame::InferenceState)
for caller in frames
caller.valid_worlds = cycle_valid_worlds
caller.ipo_effects = cycle_effects
has_cycle && cycle_ssaflags!(caller, frames, cycle_effects)
finish(caller, caller.interp)
end
for caller in frames
Expand Down
9 changes: 9 additions & 0 deletions test/compiler/inference.jl
Original file line number Diff line number Diff line change
Expand Up @@ -5291,6 +5291,15 @@ end
foo51090(b) = return bar51090(b)
@test !fully_eliminated(foo51090, (Int,))

Base.@assume_effects :terminates_globally @noinline function bar51090_terminates(b)
b == 0 && return
r = foo51090_terminates(b - 1)
Base.donotdelete(b)
return r
end
foo51090_terminates(b) = return bar51090_terminates(b)
@test !fully_eliminated(foo51090_terminates, (Int,))

# exploit throwness from concrete eval for intrinsics
@test Base.return_types() do
Base.or_int(true, 1)
Expand Down

0 comments on commit 878ca04

Please sign in to comment.