Skip to content

help bounds checking to be eliminated for getindex(::Memory, ::Int) #58754

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.

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

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented Jun 17, 2025

Second try for PR #58741.

This moves the getindex(::Memory, ::Int) bounds check to Julia, which is how it's already done for getindex(::Array, ::Int), so I guess it's correct.

Also deduplicate the bounds checking code while at it.

@nsajko
Copy link
Contributor Author

nsajko commented Jun 17, 2025

Reproducer:

function g(a::AbstractArray{Float32}, i::Int)
    @inline let
        if checkbounds(Bool, a, i)
            a[i]
        else
            Float32(0)
        end
    end
end

code_llvm(g, Tuple{Memory{Float32}, Int}; debuginfo=:none)  # there should be no more bounds checking after this PR

@nsajko nsajko added the arrays [a, r, r, a, y, s] label Jun 17, 2025
@nsajko nsajko force-pushed the bounds_checking_elimination_for_getindex_GenericMemory_second_try branch from d55a60e to c334dec Compare June 17, 2025 11:15
Second try for PR JuliaLang#58741.

This moves the `getindex(::Memory, ::Int)` bounds check to Julia, which
is how it's already done for `getindex(::Array, ::Int)`, so I guess it's
correct.

Also deduplicate the bounds checking code while at it.
@nsajko nsajko force-pushed the bounds_checking_elimination_for_getindex_GenericMemory_second_try branch from c334dec to 9dd4bda Compare June 17, 2025 11:37
@gbaraldi
Copy link
Member

@nanosoldier runbenchmarks(ALL, vs=":master")

@giordano
Copy link
Member

At a quick glance, this seems to help quite a bit for JuliaArrays/FixedSizeArrays.jl#142, but not fully close the gap.

@oscardssmith
Copy link
Member

#58768 should also hopefully help with this (once the codegen gets updated to work with this)

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@oscardssmith
Copy link
Member

benchmarks look totally meaningless (we seem to be missing any Memory benchmarks). That said, lgtm.

@nsajko
Copy link
Contributor Author

nsajko commented Jun 18, 2025

I tried to reproduce one regression and one perf improvement locally, both turned out to be noise.

@nsajko nsajko added the merge me PR is reviewed. Merge when all tests are passing label Jun 18, 2025
@nsajko
Copy link
Contributor Author

nsajko commented Jun 18, 2025

Merge? I'd like to rebase #58755 on top of this.

@oscardssmith oscardssmith merged commit 7865349 into JuliaLang:master Jun 18, 2025
9 checks passed
@nsajko nsajko deleted the bounds_checking_elimination_for_getindex_GenericMemory_second_try branch June 18, 2025 19:24
@nsajko nsajko removed the merge me PR is reviewed. Merge when all tests are passing label Jun 18, 2025
nsajko added a commit to nsajko/julia that referenced this pull request Jun 18, 2025
After PR JuliaLang#58754 the bounds checking is eliminated by LLVM even without
asserting `inbounds`, so delete `inbounds` to allow the `noub` effect
to be inferred.

Also deduplicate and test for good effect inference.
nsajko added a commit to nsajko/julia that referenced this pull request Jun 18, 2025
After PR JuliaLang#58754 the bounds checking is eliminated by LLVM even without
asserting `inbounds`, so delete `inbounds` to allow the `noub` effect
to be inferred.

Also deduplicate and test for good effect inference.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants