From e93b2b092461548c257ca83e1f61c02349cdb142 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 | 10 ++++----- base/experimental.jl | 4 ++-- base/tuple.jl | 4 ++-- src/cgutils.cpp | 27 ++----------------------- src/codegen.cpp | 4 ++-- 11 files changed, 28 insertions(+), 53 deletions(-) diff --git a/base/array.jl b/base/array.jl index 3a12b38c5bc26..12f9443a8ef21 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 6698d4360cc7d..43d93ce8900eb 100644 --- a/base/boot.jl +++ b/base/boot.jl @@ -852,4 +852,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 6e30c9991e45e..f7fe7b2d3fad7 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 3aa2366b48aa3..e55c600a28484 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 17df27bd5f637..875e38a04fa15 100644 --- a/base/compiler/ssair/inlining.jl +++ b/base/compiler/ssair/inlining.jl @@ -664,8 +664,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 f3c5694535ce6..79e21f9b9f14c 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 0477ccd5b172a..c526bfc27cba8 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 @@ -678,7 +678,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 """ @@ -741,7 +741,7 @@ end # SimpleVector -@eval getindex(v::SimpleVector, i::Int) = (@_foldable_meta; Core._svec_ref($(Expr(:boundscheck)), v, i)) +@eval getindex(v::SimpleVector, i::Int) = (@_foldable_meta; Core._svec_ref(should_check_bounds($(Expr(:boundscheck))), v, i)) function length(v::SimpleVector) @_total_meta t = @_gc_preserve_begin v diff --git a/base/experimental.jl b/base/experimental.jl index cc8d368023b49..4a7b952ede9a9 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 59fe2c1e531e1..33aa947c5fd82 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 8442ba99bb411..c640d391d98e6 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; } @@ -2960,14 +2943,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 @@ -2979,7 +2960,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 @@ -2989,12 +2969,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. @@ -3055,7 +3033,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 137d3d78814af..40ac79141c1b4 100644 --- a/src/codegen.cpp +++ b/src/codegen.cpp @@ -3793,7 +3793,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 { @@ -5713,7 +5713,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);