From 5d5a5b6158d1ec52c5627258247c78b336eadea0 Mon Sep 17 00:00:00 2001 From: Keno Fischer Date: Mon, 2 Jul 2018 20:30:11 -0400 Subject: [PATCH] Use correct effect_free predicate in inlining As part of the new optimizer work, I wrote a more accurate effect_free predicate (stmt_effect_free). However, the old optimizer's less accurate predicate stuck around and snuck into two places 1) The inliner 2) The optimize.jl code that computes proven_pure As a result, the compiler would eliminate certain statements, even if it could not prove that they were effect_free. Fix all that by getting rid of the old predicate and using the new one everywhere. Fixes #27910 --- base/compiler/optimize.jl | 128 +++----------------------------- base/compiler/ssair/inlining.jl | 2 +- base/compiler/ssair/queries.jl | 2 +- test/core.jl | 4 + 4 files changed, 18 insertions(+), 118 deletions(-) diff --git a/base/compiler/optimize.jl b/base/compiler/optimize.jl index 1a3cddbffcbf7..d7c81db97445a 100644 --- a/base/compiler/optimize.jl +++ b/base/compiler/optimize.jl @@ -144,7 +144,6 @@ function optimize(opt::OptimizationState, @nospecialize(result)) nargs = Int(opt.nargs) - 1 @timeit "optimizer" ir = run_passes(opt.src, nargs, opt) force_noinline = _any(@nospecialize(x) -> isexpr(x, :meta) && x.args[1] == :noinline, ir.meta) - replace_code_newstyle!(opt.src, ir, nargs) # compute inlining and other related optimizations if (isa(result, Const) || isconstType(result)) @@ -153,10 +152,17 @@ function optimize(opt::OptimizationState, @nospecialize(result)) # (issue #20704) # TODO: Improve this analysis; if a function is marked @pure we should really # only care about certain errors (e.g. method errors and type errors). - if length(opt.src.code) < 10 + if length(ir.stmts) < 10 proven_pure = true - for i in 1:length(opt.src.code) - if !statement_effect_free(opt.src.code[i], opt, opt.src.ssavaluetypes[i]) + for i in 1:length(ir.stmts) + stmt = ir.stmts[i] + # These affect control flow within the function (so may not be removed + # if there is no usage within the function), but don't affect the purity + # of the function as a whole. + if isa(stmt, GotoIfNot) || isa(stmt, GotoNode) || isa(stmt, ReturnNode) + continue + end + if !stmt_effect_free(stmt, ir, ir.spvals) proven_pure = false break end @@ -188,6 +194,8 @@ function optimize(opt::OptimizationState, @nospecialize(result)) end end + replace_code_newstyle!(opt.src, ir, nargs) + # determine and cache inlineability if !force_noinline # don't keep ASTs for functions specialized on a Union argument @@ -252,118 +260,6 @@ function is_pure_builtin(@nospecialize(f)) end end -function statement_effect_free(@nospecialize(e), me::OptimizationState, @nospecialize(etype)) - if isa(e, Expr) - if e.head === :(=) - return !isa(e.args[1], GlobalRef) && effect_free(e.args[2], me, false, etype) - elseif e.head === :gotoifnot - return effect_free(e.args[1], me, false, etype) - end - elseif isa(e, GotoNode) - return true - end - return effect_free(e, me, false, etype) -end - -effect_free(@nospecialize(e), s::OptimizationState, allow_volatile::Bool, @nospecialize(etype)) = - effect_free(e, s.src, s.sp, allow_volatile, etype, s.slottypes) - -# detect some important side-effect-free calls (allow_volatile=true) -# and some affect-free calls (allow_volatile=false) -- affect_free means the call -# cannot be affected by previous calls, except assignment nodes -function effect_free(@nospecialize(e), src, spvals::SimpleVector, allow_volatile::Bool, @nospecialize(etype), - slottypes::Vector{Any} = empty_slottypes) - if isa(e, GlobalRef) - return (isdefined(e.mod, e.name) && (allow_volatile || isconst(e.mod, e.name))) - elseif isa(e, Slot) - return src.slotflags[slot_id(e)] & SLOT_USEDUNDEF == 0 - elseif isa(e, Expr) - e = e::Expr - head = e.head - if is_meta_expr_head(head) - return true - end - if head === :static_parameter - # if we aren't certain enough about the type, it might be an UndefVarError at runtime - return isa(etype, Const) || issingletontype(widenconst(etype)) - end - if etype === Bottom - return false - end - ea = e.args - if head === :call - if is_known_call_p(e, is_pure_builtin, src, spvals, slottypes) - if !allow_volatile - if is_known_call(e, arrayref, src, spvals, slottypes) || is_known_call(e, arraylen, src, spvals, slottypes) - return false - elseif is_known_call(e, getfield, src, spvals, slottypes) - nargs = length(ea) - (3 <= nargs <= 4) || return false - # TODO: check ninitialized - if !isa(etype, Const) && !isconstType(etype) - # first argument must be immutable to ensure e is affect_free - a = ea[2] - typ = unwrap_unionall(widenconst(argextype(a, src, spvals, slottypes))) - if isType(typ) - # all fields of subtypes of Type are effect-free - # (including the non-inferrable uid field) - elseif !isa(typ, DataType) || typ.abstract || (typ.mutable && length(typ.types) > 0) - return false - end - end - end - end - # fall-through - elseif is_known_call(e, _apply, src, spvals, slottypes) && length(ea) > 1 - ft = argextype(ea[2], src, spvals, slottypes) - if !isa(ft, Const) || (!contains_is(_PURE_BUILTINS, ft.val) && - ft.val !== Core.sizeof) - return false - end - # fall-through - else - return false - end - elseif head === :new - a = ea[1] - typ = argextype(a, src, spvals, slottypes) - # `Expr(:new)` of unknown type could raise arbitrary TypeError. - typ, isexact = instanceof_tfunc(typ) - isexact || return false - isconcretedispatch(typ) || return false - typ = typ::DataType - if !allow_volatile && typ.mutable - return false - end - fieldcount(typ) >= length(ea) - 1 || return false - for fld_idx in 1:(length(ea) - 1) - eT = argextype(ea[fld_idx + 1], src, spvals, slottypes) - fT = fieldtype(typ, fld_idx) - eT ⊑ fT || return false - end - # fall-through - elseif head === :return - # fall-through - elseif head === :isdefined - return allow_volatile - elseif head === :the_exception - return allow_volatile - elseif head === :copyast - return true - else - return false - end - for a in ea - if !effect_free(a, src, spvals, allow_volatile, argextype(a, src, spvals, slottypes)) - return false - end - end - elseif isa(e, GotoNode) - return false - end - return true -end - ## Computing the cost of a function body # saturating sum (inputs are nonnegative), prevents overflow with typemax(Int) below diff --git a/base/compiler/ssair/inlining.jl b/base/compiler/ssair/inlining.jl index 421701c6b257d..5556108fc1bad 100644 --- a/base/compiler/ssair/inlining.jl +++ b/base/compiler/ssair/inlining.jl @@ -1027,7 +1027,7 @@ function early_inline_special_case(ir::IRCode, @nospecialize(f), @nospecialize(f (f === Core.kwfunc && length(atypes) == 2) || (is_inlineable_constant(val) && (contains_is(_PURE_BUILTINS, f) || - (f === getfield && effect_free(e, ir, ir.spvals, false, etype)) || + (f === getfield && stmt_effect_free(e, ir, ir.spvals)) || (isa(f, IntrinsicFunction) && is_pure_intrinsic_optim(f))))) return quoted(val) end diff --git a/base/compiler/ssair/queries.jl b/base/compiler/ssair/queries.jl index 90898deb4c0f7..421e801694001 100644 --- a/base/compiler/ssair/queries.jl +++ b/base/compiler/ssair/queries.jl @@ -3,7 +3,7 @@ """ Determine whether a statement is side-effect-free, i.e. may be removed if it has no uses. """ -function stmt_effect_free(@nospecialize(stmt), src::IncrementalCompact, spvals::SimpleVector) +function stmt_effect_free(@nospecialize(stmt), src, spvals::SimpleVector) isa(stmt, Union{PiNode, PhiNode}) && return true isa(stmt, Union{ReturnNode, GotoNode, GotoIfNot}) && return false isa(stmt, GlobalRef) && return isdefined(stmt.mod, stmt.name) diff --git a/test/core.jl b/test/core.jl index bc2abd188b666..04a5eb2928f82 100644 --- a/test/core.jl +++ b/test/core.jl @@ -6270,3 +6270,7 @@ foo27770() = get27770(Nullable27770(), Handle27770()) bar27770() = Nullable27770().value @test_throws UndefRefError bar27770() + +# Issue 27910 +f27910() = ((),)[2] +@test_throws BoundsError f27910()