From 189b6eb856d20127a9cc7b20cdc5938eea84a5df Mon Sep 17 00:00:00 2001 From: Keno Fischer Date: Tue, 20 Jun 2023 21:07:38 +0000 Subject: [PATCH] RFC: A path forward on --check-bounds In 1.9, `--check-bounds=no` has started causing significant performance regressions (e.g. #50110). This is because we switched a number of functions that used to be `@pure` to new effects-based infrastructure, which very closely tracks the the legality conditions for concrete evaluation. Unfortunately, disabling bounds checking completely invalidates all prior legality analysis, so the only realistic path we have is to completely disable it. In general, we are learning that these kinds of global make-things-faster-but-unsafe flags are highly problematic for a language for several reasons: - Code is written with the assumption of a particular mode being chosen, so it is in general not possible or unsafe to compose libraries (which in a language like julia is a huge problem). - Unsafe semantics are often harder for the compiler to reason about, causing unexpected performance issues (although the 1.9 --check-bounds=no issues are worse than just disabling concrete eval for things that use bounds checking) In general, I'd like to remove the `--check-bounds=` option entirely (#48245), but that proposal has encountered major opposition. This PR implements an alternative proposal: We introduce a new function `Core.should_check_bounds(boundscheck::Bool) = boundscheck`. This function is passed the result of `Expr(:boundscheck)` (which is now purely determined by the inliner based on `@inbounds`, without regard for the command line flag). In this proposal, what the command line flag does is simply redefine this function to either `true` or `false` (unconditionally) depending on the value of the flag. Of course, this causes massive amounts of recompilation, but I think this can be addressed by adding logic to loading that loads a pkgimage with appropriate definitions to cure the invalidations. The special logic we have now now to take account of the --check-bounds flag in .ji selection, would be replaced by automatically injecting the special pkgimage as a dependency to every loaded image. This part isn't implemented in this PR, but I think it's reasonable to do. I think with that, the `--check-bounds` flag remains functional, while having much more well defined behavior, as it relies on the standard world age mechanisms. A major benefit of this approach is that it can be scoped appropriately using overlay tables. For exmaple: ``` julia> using CassetteOverlay julia> @MethodTable AssumeInboundsTable; julia> @overlay AssumeInboundsTable Core.should_check_bounds(b::Bool) = false; julia> assume_inbounds = @overlaypass AssumeInboundsTable julia> assume_inbounds(f, args...) # f(args...) with bounds checking disabled dynamically ``` Similar logic applies to GPUCompiler, which already supports overlay tables. --- base/array.jl | 4 ++-- base/boot.jl | 2 ++ base/client.jl | 9 +++++++++ base/compiler/abstractinterpretation.jl | 7 ------- base/compiler/ssair/inlining.jl | 4 ++-- base/compiler/utilities.jl | 6 ------ base/essentials.jl | 8 ++++---- base/experimental.jl | 4 ++-- base/tuple.jl | 4 ++-- src/cgutils.cpp | 27 ++----------------------- src/codegen.cpp | 4 ++-- 11 files changed, 27 insertions(+), 52 deletions(-) diff --git a/base/array.jl b/base/array.jl index d3d4750743a917..4c8e4078c228d6 100644 --- a/base/array.jl +++ b/base/array.jl @@ -1017,9 +1017,9 @@ Dict{String, Int64} with 2 entries: function setindex! end @eval setindex!(A::Array{T}, x, i1::Int) where {T} = - arrayset($(Expr(:boundscheck)), A, x isa T ? x : convert(T,x)::T, i1) + arrayset(Core.should_check_bounds($(Expr(:boundscheck))), A, x isa T ? x : convert(T,x)::T, i1) @eval setindex!(A::Array{T}, x, i1::Int, i2::Int, I::Int...) where {T} = - (@inline; arrayset($(Expr(:boundscheck)), A, x isa T ? x : convert(T,x)::T, i1, i2, I...)) + (@inline; arrayset(Core.should_check_bounds($(Expr(:boundscheck))), A, x isa T ? x : convert(T,x)::T, i1, i2, I...)) __inbounds_setindex!(A::Array{T}, x, i1::Int) where {T} = arrayset(false, A, convert(T,x)::T, i1) diff --git a/base/boot.jl b/base/boot.jl index 78b7daaf47d64b..8a1bd21eb4aed1 100644 --- a/base/boot.jl +++ b/base/boot.jl @@ -854,4 +854,6 @@ function _hasmethod(@nospecialize(tt)) # this function has a special tfunc return Intrinsics.not_int(ccall(:jl_gf_invoke_lookup, Any, (Any, Any, UInt), tt, nothing, world) === nothing) end +should_check_bounds(boundscheck::Bool) = boundscheck + ccall(:jl_set_istopmod, Cvoid, (Any, Bool), Core, true) diff --git a/base/client.jl b/base/client.jl index 6e30c9991e45ef..f7fe7b2d3fad71 100644 --- a/base/client.jl +++ b/base/client.jl @@ -272,6 +272,15 @@ function exec_options(opts) invokelatest(Main.Distributed.process_opts, opts) end + # Maybe redefine bounds checking if requested + if JLOptions().check_bounds != 0 + if JLOptions().check_bounds == 1 + Core.eval(Main, :(Core.should_check_bounds(boundscheck::Bool) = true)) + else + Core.eval(Main, :(Core.should_check_bounds(boundscheck::Bool) = false)) + end + end + interactiveinput = (repl || is_interactive::Bool) && isa(stdin, TTY) is_interactive::Bool |= interactiveinput diff --git a/base/compiler/abstractinterpretation.jl b/base/compiler/abstractinterpretation.jl index ed41b43ff95d93..0ec0fb21839b21 100644 --- a/base/compiler/abstractinterpretation.jl +++ b/base/compiler/abstractinterpretation.jl @@ -835,13 +835,6 @@ end function concrete_eval_eligible(interp::AbstractInterpreter, @nospecialize(f), result::MethodCallResult, arginfo::ArgInfo, sv::AbsIntState) (;effects) = result - if inbounds_option() === :off - if !is_nothrow(effects) - # Disable concrete evaluation in `--check-bounds=no` mode, - # unless it is known to not throw. - return :none - end - end if !effects.noinbounds && stmt_taints_inbounds_consistency(sv) # If the current statement is @inbounds or we propagate inbounds, the call's consistency # is tainted and not consteval eligible. diff --git a/base/compiler/ssair/inlining.jl b/base/compiler/ssair/inlining.jl index cdbfaf64856993..98125e61380e85 100644 --- a/base/compiler/ssair/inlining.jl +++ b/base/compiler/ssair/inlining.jl @@ -655,8 +655,8 @@ function batch_inline!(ir::IRCode, todo::Vector{Pair{Int,Any}}, propagate_inboun end finish_cfg_inline!(state) - boundscheck = inbounds_option() - if boundscheck === :default && propagate_inbounds + boundscheck = :default + if propagate_inbounds boundscheck = :propagate end diff --git a/base/compiler/utilities.jl b/base/compiler/utilities.jl index f3c5694535ce60..79e21f9b9f14c7 100644 --- a/base/compiler/utilities.jl +++ b/base/compiler/utilities.jl @@ -513,9 +513,3 @@ function coverage_enabled(m::Module) end return false end -function inbounds_option() - opt_check_bounds = JLOptions().check_bounds - opt_check_bounds == 0 && return :default - opt_check_bounds == 1 && return :on - return :off -end diff --git a/base/essentials.jl b/base/essentials.jl index 68dd0c06d646f4..3fe913f029a2da 100644 --- a/base/essentials.jl +++ b/base/essentials.jl @@ -1,6 +1,6 @@ # This file is a part of Julia. License is MIT: https://julialang.org/license -import Core: CodeInfo, SimpleVector, donotdelete, compilerbarrier, arrayref +import Core: CodeInfo, SimpleVector, donotdelete, compilerbarrier, arrayref, should_check_bounds const Callable = Union{Function,Type} @@ -10,8 +10,8 @@ const Bottom = Union{} length(a::Array) = arraylen(a) # This is more complicated than it needs to be in order to get Win64 through bootstrap -eval(:(getindex(A::Array, i1::Int) = arrayref($(Expr(:boundscheck)), A, i1))) -eval(:(getindex(A::Array, i1::Int, i2::Int, I::Int...) = (@inline; arrayref($(Expr(:boundscheck)), A, i1, i2, I...)))) +eval(:(getindex(A::Array, i1::Int) = arrayref(should_check_bounds($(Expr(:boundscheck))), A, i1))) +eval(:(getindex(A::Array, i1::Int, i2::Int, I::Int...) = (@inline; arrayref(should_check_bounds($(Expr(:boundscheck))), A, i1, i2, I...)))) ==(a::GlobalRef, b::GlobalRef) = a.mod === b.mod && a.name === b.name @@ -698,7 +698,7 @@ julia> f2() implementation after you are certain its behavior is correct. """ macro boundscheck(blk) - return Expr(:if, Expr(:boundscheck), esc(blk)) + return Expr(:if, Expr(:call, GlobalRef(Core, :should_check_bounds), Expr(:boundscheck)), esc(blk)) end """ diff --git a/base/experimental.jl b/base/experimental.jl index cc8d368023b492..4a7b952ede9a9f 100644 --- a/base/experimental.jl +++ b/base/experimental.jl @@ -29,9 +29,9 @@ Base.IndexStyle(::Type{<:Const}) = IndexLinear() Base.size(C::Const) = size(C.a) Base.axes(C::Const) = axes(C.a) @eval Base.getindex(A::Const, i1::Int) = - (Base.@inline; Core.const_arrayref($(Expr(:boundscheck)), A.a, i1)) + (Base.@inline; Core.const_arrayref(Core.should_check_bounds($(Expr(:boundscheck))), A.a, i1)) @eval Base.getindex(A::Const, i1::Int, i2::Int, I::Int...) = - (Base.@inline; Core.const_arrayref($(Expr(:boundscheck)), A.a, i1, i2, I...)) + (Base.@inline; Core.const_arrayref(Core.should_check_bounds($(Expr(:boundscheck))), A.a, i1, i2, I...)) """ @aliasscope expr diff --git a/base/tuple.jl b/base/tuple.jl index 59fe2c1e531e1a..33aa947c5fd82b 100644 --- a/base/tuple.jl +++ b/base/tuple.jl @@ -28,8 +28,8 @@ firstindex(@nospecialize t::Tuple) = 1 lastindex(@nospecialize t::Tuple) = length(t) size(@nospecialize(t::Tuple), d::Integer) = (d == 1) ? length(t) : throw(ArgumentError("invalid tuple dimension $d")) axes(@nospecialize t::Tuple) = (OneTo(length(t)),) -@eval getindex(@nospecialize(t::Tuple), i::Int) = getfield(t, i, $(Expr(:boundscheck))) -@eval getindex(@nospecialize(t::Tuple), i::Integer) = getfield(t, convert(Int, i), $(Expr(:boundscheck))) +@eval getindex(@nospecialize(t::Tuple), i::Int) = getfield(t, i, should_check_bounds($(Expr(:boundscheck)))) +@eval getindex(@nospecialize(t::Tuple), i::Integer) = getfield(t, convert(Int, i), should_check_bounds($(Expr(:boundscheck)))) __inbounds_getindex(@nospecialize(t::Tuple), i::Int) = getfield(t, i, false) __inbounds_getindex(@nospecialize(t::Tuple), i::Integer) = getfield(t, convert(Int, i), false) getindex(t::Tuple, r::AbstractArray{<:Any,1}) = (eltype(t)[t[ri] for ri in r]...,) diff --git a/src/cgutils.cpp b/src/cgutils.cpp index 9b3d634c76400f..11b976b6f70a4e 100644 --- a/src/cgutils.cpp +++ b/src/cgutils.cpp @@ -1736,26 +1736,10 @@ static void emit_concretecheck(jl_codectx_t &ctx, Value *typ, const std::string error_unless(ctx, emit_isconcrete(ctx, typ), msg); } -#define CHECK_BOUNDS 1 -static bool bounds_check_enabled(jl_codectx_t &ctx, jl_value_t *inbounds) { -#if CHECK_BOUNDS==1 - if (jl_options.check_bounds == JL_OPTIONS_CHECK_BOUNDS_ON) - return 1; - if (jl_options.check_bounds == JL_OPTIONS_CHECK_BOUNDS_OFF) - return 0; - if (inbounds == jl_false) - return 0; - return 1; -#else - return 0; -#endif -} - static Value *emit_bounds_check(jl_codectx_t &ctx, const jl_cgval_t &ainfo, jl_value_t *ty, Value *i, Value *len, jl_value_t *boundscheck) { Value *im1 = ctx.builder.CreateSub(i, ConstantInt::get(ctx.types().T_size, 1)); -#if CHECK_BOUNDS==1 - if (bounds_check_enabled(ctx, boundscheck)) { + if (boundscheck != jl_false) { ++EmittedBoundschecks; Value *ok = ctx.builder.CreateICmpULT(im1, len); setName(ctx.emission_context, ok, "boundscheck"); @@ -1790,7 +1774,6 @@ static Value *emit_bounds_check(jl_codectx_t &ctx, const jl_cgval_t &ainfo, jl_v ctx.f->getBasicBlockList().push_back(passBB); ctx.builder.SetInsertPoint(passBB); } -#endif return im1; } @@ -3031,14 +3014,12 @@ static Value *emit_array_nd_index( Value *a = boxed(ctx, ainfo); Value *i = Constant::getNullValue(ctx.types().T_size); Value *stride = ConstantInt::get(ctx.types().T_size, 1); -#if CHECK_BOUNDS==1 - bool bc = bounds_check_enabled(ctx, inbounds); + bool bc = inbounds != jl_false; BasicBlock *failBB = NULL, *endBB = NULL; if (bc) { failBB = BasicBlock::Create(ctx.builder.getContext(), "oob"); endBB = BasicBlock::Create(ctx.builder.getContext(), "idxend"); } -#endif SmallVector idxs(nidxs); for (size_t k = 0; k < nidxs; k++) { idxs[k] = emit_unbox(ctx, ctx.types().T_size, argv[k], (jl_value_t*)jl_long_type); // type asserted by caller @@ -3050,7 +3031,6 @@ static Value *emit_array_nd_index( if (k < nidxs - 1) { assert(nd >= 0); Value *d = emit_arraysize_for_unsafe_dim(ctx, ainfo, ex, k + 1, nd); -#if CHECK_BOUNDS==1 if (bc) { BasicBlock *okBB = BasicBlock::Create(ctx.builder.getContext(), "ib"); // if !(i < d) goto error @@ -3060,12 +3040,10 @@ static Value *emit_array_nd_index( ctx.f->getBasicBlockList().push_back(okBB); ctx.builder.SetInsertPoint(okBB); } -#endif stride = ctx.builder.CreateMul(stride, d); setName(ctx.emission_context, stride, "stride"); } } -#if CHECK_BOUNDS==1 if (bc) { // We have already emitted a bounds check for each index except for // the last one which we therefore have to do here. @@ -3126,7 +3104,6 @@ static Value *emit_array_nd_index( ctx.f->getBasicBlockList().push_back(endBB); ctx.builder.SetInsertPoint(endBB); } -#endif return i; } diff --git a/src/codegen.cpp b/src/codegen.cpp index 41cb5ab8a891a4..90c240d0ed0567 100644 --- a/src/codegen.cpp +++ b/src/codegen.cpp @@ -3817,7 +3817,7 @@ static bool emit_builtin_call(jl_codectx_t &ctx, jl_cgval_t *ret, jl_value_t *f, assert(jl_is_datatype(jt)); // This is not necessary for correctness, but allows to omit // the extra code for getting the length of the tuple - if (!bounds_check_enabled(ctx, boundscheck)) { + if (boundscheck == jl_false) { vidx = ctx.builder.CreateSub(vidx, ConstantInt::get(ctx.types().T_size, 1)); } else { @@ -5772,7 +5772,7 @@ static jl_cgval_t emit_expr(jl_codectx_t &ctx, jl_value_t *expr, ssize_t ssaidx_ jl_errorf("Expr(:%s) in value position", jl_symbol_name(head)); } else if (head == jl_boundscheck_sym) { - return mark_julia_const(ctx, bounds_check_enabled(ctx, jl_true) ? jl_true : jl_false); + return mark_julia_const(ctx, jl_true); } else if (head == jl_gc_preserve_begin_sym) { SmallVector argv(nargs);