Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
improve the interplay between bounds checking system and effect system
In the current state of the Julia compiler, bounds checking and its related optimization code, such as `@boundscheck` and `@inbounds`, pose a significant handicap for effect analysis. As a result, we're encountering an ironic situation where the application of `@inbounds` annotations, which are intended to optimize performance, instead obstruct the program's optimization, thereby preventing us from achieving optimal performance. This PR is designed to resolve this situation. It aims to enhance the relationship between bounds checking and effect analysis, thereby correctly improving the performance of programs that have `@inbounds` annotations. In the following, I'll first explain the reasons that have led to this situation for better understanding, and then I'll present potential improvements to address these issues. This commit is a collection of various improvement proposals. It's necessary that we incorporate all of them simultaneously to enhance the situation without introducing any regressions. \## Core of the Problem There are fundamentally two reasons why effect analysis of code containing bounds checking is difficult: 1. The evaluation value of `Expr(:boundscheck)` is influenced by the `@inbounds` macro and the `--check-bounds` flag. Hence, when performing a concrete evaluation of a method containing `Expr(:boundscheck)`, it's crucial to respect the `@inbounds` macro context and the `--check-bounds` settings, ensuring the method's behavior is consistent across the compile time concrete evaluation and the runtime execution. 1. If the code, from which bounds checking has been removed due to `@inbounds` or `--check-bounds=no`, is unsafe, it may lead to undefined behavior due to uncertain memory access. \## Current State The current Julia compiler handles these two problems as follows: \### Current State 1 Regarding the first problem, if a code or method call containing `Expr(:boundscheck)` is within an `@inbounds` context, a concrete evaluation is immediately prohibited. For instance, in the following case, when analyzing `bar()`, if you simply perform concrete evaluation of `foo()`, it wouldn't properly respect the `@inbounds` context present in `bar()`. However, since the concrete evaluation of `foo()` is prohibited, it doesn't pose an issue: ```julia foo() = (r = 0; @BoundsCheck r += 1; return r) bar() = @inbounds foo() ``` Conversely, in the following case, there is _no need_ to prohibit the concrete evaluation of `A1_inbounds` due to the presence of `@inbounds`. This is because ~~the execution of the `@boundscheck` block is determined by the presence of local `@inbounds`~~ `Expr(:boundscheck)` within a local `@inbounds` context does not need to block concrete evaluation: ```julia function A1_inbounds() r = 0 @inbounds begin @BoundsCheck r += 1 end return r end ``` However, currently, we prohibit the concrete evaluation of such code as well. ~~Moreover, we are not handling such local `@inbounds` contexts effectively, which results in incorrect execution of `A1_inbounds()` (even our test is incorrect for this example: `https://github.com/JuliaLang/julia/blob/834aad4ab409f4ba65cbed2963b9ab6fa2770354/test/boundscheck_exec.jl#L34`)~~ EDIT: It is an expected behavior as pointed out by Jameson. Furthermore, there is room for improvement when the `--check-bounds` flag is specified. Specifically, when the `--check-bounds` flag is set, the evaluation value of `Expr(:boundscheck)` is determined irrespective of the `@inbounds` context. Hence, there is no need to prohibit concrete evaluations due to inconsistency in the evaluation value of `Expr(:boundscheck)`. \### Current State 2 Next, we've ensured that concrete evaluation isn't performed when there's potentially unsafe code that may have bounds checking removed, or when the `--check-bounds=no` flag is set, which could lead to bounds checking being removed always. For instance, if you perform concrete evaluation for the function call `baz((1,2,3), 4)` in the following example, it may return a value accessed from illicit memory and introduce undefined behaviors into the program: ```julia baz(t::Tuple, i::Int) = @inbounds t[i] baz((1,2,3), 4) ``` However, it's evident that the above code is incorrect and unsafe program and I believe undefined behavior in such programs is deemed, as explicitly stated in the `@inbounds` documentation: > │ Warning > │ > │ Using @inbounds may return incorrect results/crashes/corruption for > │ out-of-bounds indices. The user is responsible for checking it > │ manually. Only use @inbounds when it is certain from the information > │ locally available that all accesses are in bounds. Actually, the `@inbounds` macro is primarily an annotation to "improve performance by removing bounds checks from safe programs". Therefore, I opine that it would be more reasonable to utilize it to alleviate potential errors due to bounds checking within `@inbounds` contexts. To bring up another associated concern, in the current compiler implementation, the `:nothrow` modelings for `getfield`/`arrayref`/`arrayset` is a bit risky, and `:nothrow`-ness is assumed when their bounds checking is turned off by call argument. If our intended direction aligns with the removal of bounds checking based on `@inbounds` as proposed in issue #48245, then assuming `:nothrow`-ness due to `@inbounds` seems reasonable. However, presuming `:nothrow`-ness due to bounds checking argument or the `--check-bounds` flag appears to be risky, especially considering it's not documented. \## This Commit This commit implements all proposed improvements against the current issues as mentioned above. In summary, the enhancements include: - making `Expr(:boundscheck)` within a local `@inbounds` context not block concrete evaluation - folding out `Expr(:boundscheck)` when the `--check-bounds` flag is set (and allow concrete evaluation) - changing the `:nothrow` effect bit to `UInt8` type, and refining `:nothrow` information when in an `@inbounds` context - removing dangerous assumptions of `:nothrow`-ness for built-in functions when bounds checking is turned off - replacing the `@_safeindex` hack with `@inbounds`
- Loading branch information
