Skip to content
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

eliminate the dead iterate branch in _unsafe_(get)setindex!. #52809

Merged
merged 3 commits into from
Feb 13, 2024

Conversation

N5N3
Copy link
Member

@N5N3 N5N3 commented Jan 8, 2024

It's sad that compiler can't do this automatically.
Some benchmark with setindex!:

julia> a = zeros(Int, 100, 100);
julia> @btime $a[:,:] = $(1:10000);
  1.340 μs (0 allocations: 0 bytes) #master: 3.350 μs (0 allocations: 0 bytes)

julia> @btime $a[:,:] = $(view(LinearIndices(a), 1:100, 1:100));
  10.000 μs (0 allocations: 0 bytes) #master: 11.000 μs (0 allocations: 0 bytes)

So at least IndexLinear case get vectorized, which should be good for getindex as most of it results are IndexLinear.
Edit: Oops, just found that setindex! use an AbstractArray-based iteration. So the acceleration is quite limited.
Since #51071 we can't assuming all AbstractArray supports indexing, so switch to eachindex based iteration might be breaking.

BTW optimization for FastSubArray introduced in #45371 still work after this change as the parent array might have their own copyto! optimization.

@adienes
Copy link
Contributor

adienes commented Jan 8, 2024

Since #51071 we can't assuming all AbstractArray supports indexing

I mean, getindex is in the interface, so I think you can assume that.
if something subtypes AbstractArray without implementing getindex it's a bug on their side

@N5N3
Copy link
Member Author

N5N3 commented Jan 8, 2024

setindex! now use eachindex based iteration.
Update the latest benchmark:

julia> a = zeros(Int, 100, 100);
julia> @btime $a[:,:] = $(1:10000);
  1.410 μs (0 allocations: 0 bytes) #master: 3.350 μs (0 allocations: 0 bytes)

julia> @btime $a[:,:] = $(view(LinearIndices(a), 1:100, 1:100));
  7.475 μs (0 allocations: 0 bytes) #master: 11.000 μs (0 allocations: 0 bytes)

@N5N3 N5N3 added the needs pkgeval Tests for all registered packages should be run with this change label Jan 8, 2024
@N5N3
Copy link
Member Author

N5N3 commented Jan 9, 2024

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@N5N3 N5N3 removed the needs pkgeval Tests for all registered packages should be run with this change label Jan 9, 2024
@jishnub
Copy link
Contributor

jishnub commented Jan 11, 2024

There appears to be a mild regression on this PR:

julia> using LinearAlgebra

julia> U = UpperTriangular(rand(100,100)); C = similar(U);

julia> @btime $C .= 2.0 .* $U;
  1.867 μs (0 allocations: 0 bytes) # master, commit 2afc20c18a8
  2.166 μs (0 allocations: 0 bytes) # PR, N5N3:simd_index/83cb107971f

julia> versioninfo()
Julia Version 1.11.0-DEV.1233
Commit 83cb107971f (2024-01-08 14:47 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 8 × 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz
  WORD_SIZE: 64
  LLVM: libLLVM-15.0.7 (ORCJIT, tigerlake)
Threads: 1 default, 0 interactive, 1 GC (on 8 virtual cores)
Environment:
  LD_LIBRARY_PATH = :/usr/lib/x86_64-linux-gnu/gtk-3.0/modules
  JULIA_EDITOR = subl

@N5N3
Copy link
Member Author

N5N3 commented Jan 11, 2024

IIUC, this pr didn't touch the broadcast (and scalar indexing) so this looks like a bench noise, or an improvement comes from LLVM upgrade (assuming you use the latest master)

@jishnub
Copy link
Contributor

jishnub commented Feb 1, 2024

Gentle bump, is there work that remains in this?

Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Should we call this _unchecked_iterate though, to indicate that it doesn't check for termination, or _prechecked_iterate to indicate the caller is supposed to have checked, instead of _checked_iterate?

base/multidimensional.jl Outdated Show resolved Hide resolved
N5N3 and others added 3 commits February 12, 2024 14:51
by eliminating the dead branch in `iterate` manually.
renaming to `_prechecked_iterate`

Co-Authored-By: Jameson Nash <[email protected]>
@vtjnash vtjnash added the status:merge me PR is reviewed. Merge when all tests are passing label Feb 12, 2024
@vtjnash vtjnash merged commit 75cb2a5 into JuliaLang:master Feb 13, 2024
5 of 8 checks passed
@oscardssmith oscardssmith removed the status:merge me PR is reviewed. Merge when all tests are passing label Feb 13, 2024
@N5N3 N5N3 deleted the simd_index branch February 23, 2024 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s] performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants