Skip to content

Look through SSAValue to find include #869

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
Keno merged 1 commit into from
Dec 5, 2024
Merged

Look through SSAValue to find include #869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 19 additions & 7 deletions src/lowered.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
add_includes!(methodinfo::MethodInfo, mod::Module, filename) = methodinfo

function is_some_include(@nospecialize(f))
@assert !isa(f, Core.SSAValue) && !isa(f, JuliaInterpreter.SSAValue)
if isa(f, GlobalRef)
return f.name === :include
elseif isa(f, Symbol)
Expand All @@ -44,17 +45,21 @@
end

# This is not generally used, see `is_method_or_eval` instead
function hastrackedexpr(stmt; heads=LoweredCodeUtils.trackedheads)
function hastrackedexpr(stmt, code; heads=LoweredCodeUtils.trackedheads)
haseval = false
if isa(stmt, Expr)
haseval = matches_eval(stmt)
if stmt.head === :call
f = stmt.args[1]
while isa(f, Core.SSAValue) || isa(f, JuliaInterpreter.SSAValue)
f = code[f.id]
end

Check warning on line 56 in src/lowered.jl

View check run for this annotation

Codecov / codecov/patch

src/lowered.jl#L54-L56 

Added lines #L54 - L56 were not covered by tests
Comment on lines +54 to +56
Copy link
Collaborator

@aviatesk aviatesk Dec 5, 2024

Choose a reason for hiding this comment

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

We have quite a bit of similar code in JuliaInterpreter and LoweredCodeUtils, so it might be a good idea to consolidate it into a single utility.
Other code typically doesn’t use while like this but instead only follows a single level of SSAValue references. I’ll handle the refactoring later.

Copy link
Collaborator Author

@Keno Keno Dec 5, 2024

Choose a reason for hiding this comment

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

There's lots of hacks all over the place. I think we need to take a much wider look at refactoring this stack next year, probably once JuliaLowering is ready. It's just all way too ad hoc and fragile

Copy link
Collaborator

@aviatesk aviatesk Dec 5, 2024

Choose a reason for hiding this comment

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

Yeah overhauling the whole stack at once with integrating JuliaLowering would be very reasonable.

Copy link
Owner

@timholy timholy Dec 28, 2024

Choose a reason for hiding this comment

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

Yes, once JuliaLowering is available we can make everything sane.

callee_matches(f, Core, :_typebody!) && return true, haseval
callee_matches(f, Core, :_setsuper!) && return true, haseval
is_some_include(f) && return true, haseval
elseif stmt.head === :thunk
any(s->any(hastrackedexpr(s; heads=heads)), (stmt.args[1]::Core.CodeInfo).code) && return true, haseval
newcode = (stmt.args[1]::Core.CodeInfo).code
any(s->any(hastrackedexpr(s, newcode; heads=heads)), newcode) && return true, haseval
elseif stmt.head ∈ heads
return true, haseval
end
Expand All @@ -70,14 +75,21 @@
(isa(f, GlobalRef) && f.name === :eval) || is_quotenode_egal(f, Core.eval)
end

function categorize_stmt(@nospecialize(stmt))
function categorize_stmt(@nospecialize(stmt), code::Vector{Any})
ismeth, haseval, isinclude, isnamespace, istoplevel = false, false, false, false, false
if isa(stmt, Expr)
haseval = matches_eval(stmt)
ismeth = stmt.head === :method || (stmt.head === :thunk && defines_function(only(stmt.args)))
istoplevel = stmt.head === :toplevel
isnamespace = stmt.head === :export || stmt.head === :import || stmt.head === :using
isinclude = stmt.head === :call && is_some_include(stmt.args[1])
isinclude = false
if stmt.head === :call && length(stmt.args) >= 1
callee = stmt.args[1]
while isa(callee, Core.SSAValue) || isa(callee, JuliaInterpreter.SSAValue)
callee = code[callee.id]
end
isinclude = is_some_include(callee)
end
end
return ismeth, haseval, isinclude, isnamespace, istoplevel
end
Expand Down Expand Up @@ -112,7 +124,7 @@
evalassign = false
for (i, stmt) in enumerate(src.code)
if !isrequired[i]
isrequired[i], haseval = predicate(stmt)::Tuple{Bool,Bool}
isrequired[i], haseval = predicate(stmt, src.code)::Tuple{Bool,Bool}
if haseval # line `i` may be the equivalent of `f = Core.eval`, so...
isrequired[edges.succs[i]] .= true # ...require each stmt that calls `eval` via `f(expr)`
isrequired[i] = true
Expand Down Expand Up @@ -169,8 +181,8 @@
minimal_evaluation!(predicate, methodinfo, moduleof(frame), frame.framecode.src, mode)

function minimal_evaluation!(methodinfo, frame::JuliaInterpreter.Frame, mode::Symbol)
minimal_evaluation!(methodinfo, frame, mode) do @nospecialize(stmt)
ismeth, haseval, isinclude, isnamespace, istoplevel = categorize_stmt(stmt)
minimal_evaluation!(methodinfo, frame, mode) do @nospecialize(stmt), code
ismeth, haseval, isinclude, isnamespace, istoplevel = categorize_stmt(stmt, code)
isreq = ismeth | isinclude | istoplevel
return mode === :sigs ? (isreq, haseval) : (isreq | isnamespace, haseval)
end
Expand Down
26 changes: 17 additions & 9 deletions src/packagedef.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,15 +447,23 @@ push_expr!(methodinfo::CodeTrackingMethodInfo, mod::Module, ex::Expr) = (push!(m
pop_expr!(methodinfo::CodeTrackingMethodInfo) = (pop!(methodinfo.exprstack); methodinfo)
function add_dependencies!(methodinfo::CodeTrackingMethodInfo, edges::CodeEdges, src, musteval)
isempty(src.code) && return methodinfo
stmt1 = first(src.code)
if isa(stmt1, Core.GotoIfNot) && (dep = stmt1.cond; isa(dep, Union{GlobalRef,Symbol}))
# This is basically a hack to look for symbols that control definition of methods via a conditional.
# It is aimed at solving #249, but this will have to be generalized for anything real.
for (stmt, me) in zip(src.code, musteval)
me || continue
if hastrackedexpr(stmt)[1]
push!(methodinfo.deps, dep)
break
for i = 1:length(src.code)
stmt = src.code[i]
if isa(stmt, Core.GotoIfNot)
dep = stmt.cond
while (isa(dep, Core.SSAValue) || isa(dep, JuliaInterpreter.SSAValue))
dep = src.code[dep.id]
end
if isa(dep, Union{GlobalRef,Symbol})
# This is basically a hack to look for symbols that control definition of methods via a conditional.
# It is aimed at solving #249, but this will have to be generalized for anything real.
for (stmt, me) in zip(src.code, musteval)
me || continue
if hastrackedexpr(stmt, src.code)[1]
push!(methodinfo.deps, dep)
break
end
end
end
end
end
Expand Down
5 changes: 4 additions & 1 deletion test/backedges.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ do_test("Backedges") && @testset "Backedges" begin
src2 = src.code[idtype].args[1]
methodinfo = Revise.MethodInfo()
isrequired = Revise.minimal_evaluation!(methodinfo, frame, :sigs)[1]
@test sum(isrequired) == length(src.code)-count(e->isexpr(e, :latestworld), src.code)-1 # skips the `return` at the end
laststmt = src.code[end]
@assert isa(laststmt, Core.ReturnNode)
to_skip = isa(laststmt.val, Revise.JuliaInterpreter.SSAValue) ? 2 : 1
@test sum(isrequired) == length(src.code)-count(e->isexpr(e, :latestworld), src.code)-to_skip # skips the `return` at the end (and its argument)

src = """
# issue #249
Expand Down
Loading