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

Revert "make simd loop over ranges perform better (#28166)" #45230

Merged
merged 1 commit into from
May 10, 2022

Conversation

LilithHafner
Copy link
Member

Arguably it would be better to try to figure out why it cannot be folded but it is a quite deep chain of calls...

I think someone somewhere has fixed this upstream since 2018. To whom it may concern, thanks!

julia> using BenchmarkTools

julia> function perf_sumlinear_view(A)
           s = zero(eltype(A))
           @inbounds @simd for I in 1:length(A)
               val = view(A, I)
               s += val[]
           end
           return s
       end
perf_sumlinear_view (generic function with 1 method)

julia> A = 1:1000000
1:1000000

julia> B = 1:2:1000000
1:2:999999

julia> C = LinRange(1, 1000, 1000000)
1000000-element LinRange{Float64, Int64}:
 1.0, 1.001, 1.002, 1.003, 1.004, 1.005, 1.00599, 1.00699, 1.00799, 1.00899, 1.00999, , 999.993, 999.994, 999.995, 999.996, 999.997, 999.998, 999.999, 1000.0

julia> @btime perf_sumlinear_view(A)
  25.040 ns (1 allocation: 16 bytes)
500000500000

julia> @btime perf_sumlinear_view(B)
  5.569 ms (1 allocation: 16 bytes)
250000000000

julia> @btime perf_sumlinear_view(C)
  763.762 μs (1 allocation: 16 bytes)
5.005e8

julia> Base.delete_method(@which firstindex(A))

julia> Base.delete_method(@which firstindex(B))

julia> Base.delete_method(@which firstindex(C))

julia> @btime perf_sumlinear_view(A)
  24.878 ns (1 allocation: 16 bytes)
500000500000

julia> @btime perf_sumlinear_view(B)
  5.569 ms (1 allocation: 16 bytes)
250000000000

julia> @btime perf_sumlinear_view(C)
  763.826 μs (1 allocation: 16 bytes)
5.005e8

Fixes #45223

@LilithHafner LilithHafner requested review from KristofferC and N5N3 May 8, 2022 15:33
@N5N3
Copy link
Member

N5N3 commented May 8, 2022

Reverting should be OK as a temporary fix, as the original concern has gone.
It's good to add some TODO on firstindex(::StepRange) and length(::OrdinalRange).

julia> _firstindex(x) = Base.@invoke firstindex(x::AbstractArray)
f (generic function with 1 method)

julia> @code_typed _firstindex(1:2:10)
CodeInfo(
1 ─     invoke Base.length(x::StepRange{Int64, Int64})::Int64  # we failed to omit this call
└──     return 1
) => Int64

julia> @code_warntype length(StepRange(1,Int128(1),1))
MethodInstance for length(::StepRange{Int64, Int128})
  from length(r::OrdinalRange{T}) where T<:Union{Int128, Int64, UInt128, UInt64} in Base at range.jl:713
Static Parameters
  T = Int64
Arguments
  #self#::Core.Const(length)
  r::StepRange{Int64, Int128}
Locals
  a::Union{Int128, Int64}
  diff::Int64
  s::Int128
  @_6::Bool
Body::Union{Int128, Int64}   # not stable

@vchuravy
Copy link
Member

vchuravy commented May 8, 2022

@nanosoldier runbenchmarks(!"scalar", vs=":master")

@nanosoldier
Copy link
Collaborator

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

@LilithHafner
Copy link
Member Author

This is my first time reviewing nanosoldier results, but all the regressions look like noise to me with the possible exception of ["collection", "deletion", ("Set", "Int", "filter!")]. I couldn't consistently reproduce any of the substantive regressions locally (even ["collection", "deletion", ("Set", "Int", "filter!")]).

@KristofferC KristofferC merged commit 678e0d2 into JuliaLang:master May 10, 2022
@LilithHafner LilithHafner deleted the patch-13 branch May 10, 2022 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

firstindex(r) and fist(axes(r, 1)) return different types
5 participants