Skip to content
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

Fix cfg_simplify! for empty preds and succs #41693

Closed
wants to merge 1 commit into from
Closed

Fix cfg_simplify! for empty preds and succs #41693

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
3 changes: 3 additions & 0 deletions base/compiler/compiler.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,9 @@ include("compiler/optimize.jl") # TODO: break this up further + extract utilitie
include("compiler/bootstrap.jl")
ccall(:jl_set_typeinf_func, Cvoid, (Any,), typeinf_ext_toplevel)

const _COMPILER_WORLD_AGE = ccall(:jl_get_tls_world_age, UInt, ())
compiler_world_age() = _COMPILER_WORLD_AGE
Comment on lines +140 to +141
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this OK since jl_typeinf_world is serialized? Or should I add a C function jl_get_typeinf_world?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this will work. You'll need to ask C what the world age of inference is.

Copy link
Member Author

@tkf tkf Jul 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

jl_typeinf_world is set by jl_set_typeinf_func just above. IIUC, this jl_typeinf_world is baked in the sysimage. So, isn't this age correct (though off by one) unless jl_set_typeinf_func is called again?

Of course, I'm fine with adding a proper C function but I'm curious when/how it'd be wrong.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose you're right. World ages do persist across sysimage serialization (but not incremental serialization), so this should be ok.

Copy link
Member

Choose a reason for hiding this comment

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

We could pontially replace jl_call_in_typeinf_world with this too. While normally preserving these values would be risky, this is Core.Compiler, and is loaded early enough it is often not too important, but Revise expects to be able to change this, so it might still be a bit of a nuisance.

So might be better to write this as:

function invoke_in_compiler(args...)
    return ccall(:jl_call_in_typeinf_world, Any, (Ptr{Ptr{Cvoid}}, Cint), Any[args...], length(args))
end


include("compiler/parsing.jl")
Core.eval(Core, :(_parse = Compiler.fl_parse))

Expand Down
4 changes: 2 additions & 2 deletions base/compiler/ssair/passes.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1207,12 +1207,12 @@ function cfg_simplify!(ir::IRCode)
# Compute (renamed) successors and predecessors given (renamed) block
function compute_succs(i)
orig_bb = follow_merged_succ(result_bbs[i])
return map(i -> bb_rename_succ[i], bbs[orig_bb].succs)
return Int[bb_rename_succ[i] for i in bbs[orig_bb].succs]
end
function compute_preds(i)
orig_bb = result_bbs[i]
preds = bbs[orig_bb].preds
return map(pred -> bb_rename_pred[pred], preds)
return Int[bb_rename_pred[pred] for pred in preds]
end

BasicBlock[
Expand Down
20 changes: 16 additions & 4 deletions test/compiler/irpasses.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,18 @@ using Test
using Base.Meta
using Core: PhiNode, SSAValue, GotoNode, PiNode, QuoteNode, ReturnNode, GotoIfNot

# `cfg_simplify!` is not exercised in the usual compiler pipeline. So, it is
# possible that the methods that are not defined in compiler's world age creep
# into the definition. Let's be extra careful here by testing this function in
# compiler's world age, to keep it usable in the compiler whenever we need it.
function cfg_simplify!(ir)
return Base.invoke_in_world(
Core.Compiler.compiler_world_age(),
Core.Compiler.cfg_simplify!,
ir,
)
end

# Tests for domsort

## Test that domsort doesn't mangle single-argument phis (#29262)
Expand Down Expand Up @@ -198,7 +210,7 @@ let src = code_typed(gcd, Tuple{Int, Int})[1].first
# Test that cfg_simplify doesn't mangle IR on code with loops
ir = Core.Compiler.inflate_ir(src)
Core.Compiler.verify_ir(ir)
ir = Core.Compiler.cfg_simplify!(ir)
ir = cfg_simplify!(ir)
Core.Compiler.verify_ir(ir)
end

Expand All @@ -221,7 +233,7 @@ let m = Meta.@lower 1 + 1
src.ssaflags = fill(Int32(0), nstmts)
ir = Core.Compiler.inflate_ir(src)
Core.Compiler.verify_ir(ir)
ir = Core.Compiler.cfg_simplify!(ir)
ir = cfg_simplify!(ir)
Core.Compiler.verify_ir(ir)
ir = Core.Compiler.compact!(ir)
@test length(ir.cfg.blocks) == 1 && Core.Compiler.length(ir.stmts) == 1
Expand Down Expand Up @@ -249,7 +261,7 @@ let m = Meta.@lower 1 + 1
src.ssaflags = fill(Int32(0), nstmts)
ir = Core.Compiler.inflate_ir(src)
Core.Compiler.verify_ir(ir)
ir = Core.Compiler.cfg_simplify!(ir)
ir = cfg_simplify!(ir)
Core.Compiler.verify_ir(ir)
@test length(ir.cfg.blocks) == 5
ret_2 = ir.stmts.inst[ir.cfg.blocks[3].stmts[end]]
Expand All @@ -275,7 +287,7 @@ let m = Meta.@lower 1 + 1
src.ssaflags = fill(Int32(0), nstmts)
ir = Core.Compiler.inflate_ir(src)
Core.Compiler.verify_ir(ir)
ir = Core.Compiler.cfg_simplify!(ir)
ir = cfg_simplify!(ir)
Core.Compiler.verify_ir(ir)
@test length(ir.cfg.blocks) == 1
end
Expand Down