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

Remove all @pure, does not actually fix #115. #116

Closed
wants to merge 3 commits into from
Closed

Conversation

chriselrod
Copy link
Collaborator

@chriselrod chriselrod commented Feb 3, 2021

The only remaining occurrence of pure is in the README:

> git grep -nr "pure"                                                                                                                                                                                                                                             (base)
README.md:248:`Base.@pure ismutable(array::AbstractArray) = typeof(array).mutable`, but there are a lot of cases

I have JuliaLang/julia#265 (comment) problems on the master branch:

julia> using VectorizationBase
[ Info: Precompiling VectorizationBase [3d5dd08c-fd9d-11e8-17fa-ed2836048c2f]

julia> run(`grep -nr "pure" $(dirname(pathof(VectorizationBase)))`) # all instances of `pure` are commented out
/home/chriselrod/.julia/dev/VectorizationBase/src/VectorizationBase.jl:14:# Base.@pure asvalbool(r) = Val(map(Bool, r))
/home/chriselrod/.julia/dev/VectorizationBase/src/VectorizationBase.jl:15:# Base.@pure asvalint(r) = Val(map(Int, r))
/home/chriselrod/.julia/dev/VectorizationBase/src/llvm_intrin/vbroadcast.jl:54:        # $(Expr(:meta,:pure,:inline))
Process(`grep -nr pure /home/chriselrod/.julia/dev/VectorizationBase/src`, ProcessExited(0))

julia> VectorizationBase.has_feature(::Val{:x86_64_avx512f}) = VectorizationBase.False()

julia> VectorizationBase.has_feature(::Val{:x86_64_avx2}) = VectorizationBase.False()

julia> using VectorizedRNG
[ Info: Precompiling VectorizedRNG [33b4df10-0173-11e9-2a0c-851a7edac40e]

julia> @code_typed VectorizedRNG.setrand64counter!(local_rng(), 0x01) # we have a constant offset of 130
CodeInfo(
1%1 = Base.getfield(rng, :ptr)::Ptr{UInt64}%2 = Base.bitcast(Ptr{UInt8}, %1)::Ptr{UInt8}%3 = VectorizationBase.llvmcall(("    \n\n    define void @entry(i64, i8, i64) alwaysinline {\n    top:\n        %ptr.0 = inttoptr i64 %0 to i8*\n%ptr.1 = getelementptr inbounds i8, i8* %ptr.0, i64 %2\nstore i8 %1, i8* %ptr.1, align 1\nret void\n    }\n", "entry"), VectorizationBase.Cvoid, Tuple{Ptr{UInt8}, UInt8, Int64}, %2, v, 130)::Nothing
└──      return %3
) => Nothing

julia> @code_llvm VectorizedRNG.setrand64counter!(local_rng(), 0x01) # we have a constant offset of 514
;  @ /home/chriselrod/.julia/dev/VectorizedRNG/src/xoshiro.jl:97 within `setrand64counter!'
define void @"julia_setrand64counter!_1384"([1 x i64]* nocapture nonnull readonly align 8 dereferenceable(8) %0, i8 zeroext %1) {
top:
;  @ /home/chriselrod/.julia/dev/VectorizedRNG/src/xoshiro.jl:98 within `setrand64counter!'
; ┌ @ essentials.jl:402 within `unsafe_convert'
; │┌ @ pointer.jl:30 within `convert'
    %2 = bitcast [1 x i64]* %0 to i8**
    %3 = load i8*, i8** %2, align 8
; └└
; ┌ @ /home/chriselrod/.julia/dev/VectorizationBase/src/llvm_intrin/memory_addr.jl:782 within `vstore!' @ /home/chriselrod/.julia/dev/VectorizationBase/src/llvm_intrin/memory_addr.jl:638
; │┌ @ /home/chriselrod/.julia/dev/VectorizationBase/src/llvm_intrin/memory_addr.jl:638 within `macro expansion'
    %ptr.1.i = getelementptr inbounds i8, i8* %3, i64 514
    store i8 %1, i8* %ptr.1.i, align 1
; └└
  ret void
}

The function was not updating, it still says %ptr.1.i = getelementptr inbounds i8, i8* %3, i64 514 instead of %ptr.1.i = getelementptr inbounds i8, i8* %3, i64 130.
But after this PR:

julia> using VectorizationBase
[ Info: Precompiling VectorizationBase [3d5dd08c-fd9d-11e8-17fa-ed2836048c2f]

julia> run(`grep -nr "pure" $(dirname(pathof(VectorizationBase)))`) # all instances of `pure` are commented out
/home/chriselrod/.julia/dev/VectorizationBase/src/VectorizationBase.jl:14:# Base.@pure asvalbool(r) = Val(map(Bool, r))
/home/chriselrod/.julia/dev/VectorizationBase/src/VectorizationBase.jl:15:# Base.@pure asvalint(r) = Val(map(Int, r))
/home/chriselrod/.julia/dev/VectorizationBase/src/llvm_intrin/vbroadcast.jl:54:        # $(Expr(:meta,:pure,:inline))
Process(`grep -nr pure /home/chriselrod/.julia/dev/VectorizationBase/src`, ProcessExited(0))

julia> VectorizationBase.has_feature(::Val{:x86_64_avx512f}) = VectorizationBase.False()

julia> VectorizationBase.has_feature(::Val{:x86_64_avx2}) = VectorizationBase.False()

julia> using VectorizedRNG
[ Info: Precompiling VectorizedRNG [33b4df10-0173-11e9-2a0c-851a7edac40e]

julia> @code_typed VectorizedRNG.setrand64counter!(local_rng(), 0x01) # we have a constant offset of 130
CodeInfo(
1%1 = Base.getfield(rng, :ptr)::Ptr{UInt64}%2 = Base.bitcast(Ptr{UInt8}, %1)::Ptr{UInt8}%3 = VectorizationBase.llvmcall(("    \n\n    define void @entry(i64, i8, i64) alwaysinline {\n    top:\n        %ptr.0 = inttoptr i64 %0 to i8*\n%ptr.1 = getelementptr inbounds i8, i8* %ptr.0, i64 %2\nstore i8 %1, i8* %ptr.1, align 1\nret void\n    }\n", "entry"), VectorizationBase.Cvoid, Tuple{Ptr{UInt8}, UInt8, Int64}, %2, v, 130)::Nothing
└──      return %3
) => Nothing

julia> @code_llvm VectorizedRNG.setrand64counter!(local_rng(), 0x01) # we have a constant offset of 514
;  @ /home/chriselrod/.julia/dev/VectorizedRNG/src/xoshiro.jl:97 within `setrand64counter!'
define void @"julia_setrand64counter!_1386"([1 x i64]* nocapture nonnull readonly align 8 dereferenceable(8) %0, i8 zeroext %1) {
top:
;  @ /home/chriselrod/.julia/dev/VectorizedRNG/src/xoshiro.jl:98 within `setrand64counter!'
; ┌ @ essentials.jl:402 within `unsafe_convert'
; │┌ @ pointer.jl:30 within `convert'
    %2 = bitcast [1 x i64]* %0 to i8**
    %3 = load i8*, i8** %2, align 8
; └└
; ┌ @ /home/chriselrod/.julia/dev/VectorizationBase/src/llvm_intrin/memory_addr.jl:782 within `vstore!' @ /home/chriselrod/.julia/dev/VectorizationBase/src/llvm_intrin/memory_addr.jl:638
; │┌ @ /home/chriselrod/.julia/dev/VectorizationBase/src/llvm_intrin/memory_addr.jl:638 within `macro expansion'
    %ptr.1.i = getelementptr inbounds i8, i8* %3, i64 130
    store i8 %1, i8* %ptr.1.i, align 1
; └└
  ret void
}

It updated as intended.

@codecov
Copy link

codecov bot commented Feb 3, 2021

Codecov Report

Merging #116 (b3310f9) into master (854a4b8) will decrease coverage by 2.14%.
The diff coverage is 88.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
- Coverage   84.26%   82.12%   -2.15%     
==========================================
  Files           8        8              
  Lines        1506     1415      -91     
==========================================
- Hits         1269     1162     -107     
- Misses        237      253      +16     
Impacted Files Coverage Δ
src/ArrayInterface.jl 82.07% <0.00%> (-2.75%) ⬇️
src/dimensions.jl 88.02% <86.66%> (-1.58%) ⬇️
src/static.jl 78.04% <100.00%> (-3.77%) ⬇️
src/stridelayout.jl 76.49% <100.00%> (+0.10%) ⬆️
src/ranges.jl 89.65% <0.00%> (-1.84%) ⬇️
src/indexing.jl 88.01% <0.00%> (-1.02%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 854a4b8...b3310f9. Read the comment docs.

src/static.jl Outdated Show resolved Hide resolved
@Tokazama
Copy link
Member

Tokazama commented Feb 3, 2021

Looking over these, I wonder if the issue was the use of ntuple with pure. Most of these I copied nearly verbatim from other sources, but I was probably the one that committed the heinous crime of using ntuple (for some reason I thought it was a core method, not generic).

@chriselrod
Copy link
Collaborator Author

I'm getting the problem again, despite no instances of pure in src of either VectorizationBase or ArrayInterface:

julia> using VectorizationBase
[ Info: Precompiling VectorizationBase [3d5dd08c-fd9d-11e8-17fa-ed2836048c2f]

julia> !success(run(`grep -nr "pure" $(dirname(pathof(VectorizationBase)))`, wait=false))
true

julia> VectorizationBase.has_feature(::Val{:x86_64_avx512f}) = VectorizationBase.False()

julia> VectorizationBase.has_feature(::Val{:x86_64_avx2}) = VectorizationBase.False()

julia> using VectorizedRNG
[ Info: Precompiling VectorizedRNG [33b4df10-0173-11e9-2a0c-851a7edac40e]

julia> @code_typed VectorizedRNG.setrand64counter!(local_rng(), 0x01) # we have a constant offset of 130
CodeInfo(
1%1 = Base.getfield(rng, :ptr)::Ptr{UInt64}%2 = Base.bitcast(Ptr{UInt8}, %1)::Ptr{UInt8}%3 = VectorizationBase.llvmcall(("    \n\n    define void @entry(i64, i8, i64) alwaysinline {\n    top:\n        %ptr.0 = inttoptr i64 %0 to i8*\n%ptr.1 = getelementptr inbounds i8, i8* %ptr.0, i64 %2\nstore i8 %1, i8* %ptr.1, align 1\nret void\n    }\n", "entry"), VectorizationBase.Cvoid, Tuple{Ptr{UInt8}, UInt8, Int64}, %2, v, 130)::Nothing
└──      return %3
) => Nothing

julia> @code_llvm VectorizedRNG.setrand64counter!(local_rng(), 0x01) # we have a constant offset of 514
;  @ /home/chriselrod/.julia/dev/VectorizedRNG/src/xoshiro.jl:97 within `setrand64counter!'
define void @"julia_setrand64counter!_1348"([1 x i64]* nocapture nonnull readonly align 8 dereferenceable(8) %0, i8 zeroext %1) {
top:
;  @ /home/chriselrod/.julia/dev/VectorizedRNG/src/xoshiro.jl:98 within `setrand64counter!'
; ┌ @ essentials.jl:402 within `unsafe_convert'
; │┌ @ pointer.jl:30 within `convert'
    %2 = bitcast [1 x i64]* %0 to i8**
    %3 = load i8*, i8** %2, align 8
; └└
; ┌ @ /home/chriselrod/.julia/dev/VectorizationBase/src/llvm_intrin/memory_addr.jl:782 within `vstore!' @ /home/chriselrod/.julia/dev/VectorizationBase/src/llvm_intrin/memory_addr.jl:638
; │┌ @ /home/chriselrod/.julia/dev/VectorizationBase/src/llvm_intrin/memory_addr.jl:638 within `macro expansion'
    %ptr.1.i = getelementptr inbounds i8, i8* %3, i64 514
    store i8 %1, i8* %ptr.1.i, align 1
; └└
  ret void
}

julia> using ArrayInterface

julia> !success(run(`grep -nr "pure" $(dirname(pathof(ArrayInterface)))`, wait=false))
true

@chriselrod
Copy link
Collaborator Author

chriselrod commented Feb 3, 2021

@code_typed correctly shows 130 instead of 514, but @code_llvm does not.
I don't think this can be because of @generated, as the @code_typed is correct.

@DilumAluthge
Copy link
Member

I can reproduce.

julia> using VectorizationBase

julia> !success(run(`grep -nr "pure" $(dirname(pathof(VectorizationBase)))`, wait=false))
true

julia> VectorizationBase.has_feature(::Val{:x86_64_avx512f}) = VectorizationBase.False()

julia> VectorizationBase.has_feature(::Val{:x86_64_avx2}) = VectorizationBase.False()

julia> using VectorizedRNG

julia> @code_typed VectorizedRNG.setrand64counter!(local_rng(), 0x01)
CodeInfo(
1%1 = Base.getfield(rng, :ptr)::Ptr{UInt64}%2 = Base.bitcast(Ptr{UInt8}, %1)::Ptr{UInt8}%3 = VectorizationBase.llvmcall(("    \n\n    define void @entry(i64, i8, i64) alwaysinline {\n    top:\n        %ptr.0 = inttoptr i64 %0 to i8*\n%ptr.1 = getelementptr inbounds i8, i8* %ptr.0, i64 %2\nstore i8 %1, i8* %ptr.1, align 1\nret void\n    }\n", "entry"), VectorizationBase.Cvoid, Tuple{Ptr{UInt8}, UInt8, Int64}, %2, v, 130)::Nothing
└──      return %3
) => Nothing

julia> @code_llvm VectorizedRNG.setrand64counter!(local_rng(), 0x01)
;  @ /Users/dilum/.julia/packages/VectorizedRNG/RVAIZ/src/xoshiro.jl:97 within `setrand64counter!'
define void @"julia_setrand64counter!_583"([1 x i64]* nocapture nonnull readonly align 8 dereferenceable(8) %0, i8 zeroext %1) {
top:
;  @ /Users/dilum/.julia/packages/VectorizedRNG/RVAIZ/src/xoshiro.jl:98 within `setrand64counter!'
; ┌ @ essentials.jl:402 within `unsafe_convert'
; │┌ @ pointer.jl:30 within `convert'
    %2 = bitcast [1 x i64]* %0 to i8**
    %3 = load i8*, i8** %2, align 8
; └└
; ┌ @ /Users/dilum/.julia/packages/VectorizationBase/NMmk1/src/llvm_intrin/memory_addr.jl:782 within `vstore!' @ /Users/dilum/.julia/packages/VectorizationBase/NMmk1/src/llvm_intrin/memory_addr.jl:638
; │┌ @ /Users/dilum/.julia/packages/VectorizationBase/NMmk1/src/llvm_intrin/memory_addr.jl:638 within `macro expansion'
    %ptr.1.i = getelementptr inbounds i8, i8* %3, i64 258
    store i8 %1, i8* %ptr.1.i, align 1
; └└
  ret void
}

julia> using ArrayInterface

julia> !success(run(`grep -nr "pure" $(dirname(pathof(ArrayInterface)))`, wait=false))
true

@DilumAluthge
Copy link
Member

I can repro even if I do the method redefinition after using VectorizedRNG.

dilum:/Users/dilum/Downloads$ julia
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.7.0-DEV.400 (2021-01-28)
 _/ |\__'_|_|_|\__'_|  |  Commit 9de107a63a (5 days old master)
|__/                   |

julia> using VectorizationBase

julia> !success(run(`grep -nr "pure" $(dirname(pathof(VectorizationBase)))`, wait=false))
true

julia> using VectorizedRNG

julia> VectorizationBase.has_feature(::Val{:x86_64_avx512f}) = VectorizationBase.False()

julia> VectorizationBase.has_feature(::Val{:x86_64_avx2}) = VectorizationBase.False()

julia> @code_typed VectorizedRNG.setrand64counter!(local_rng(), 0x01)
CodeInfo(
1 ─ %1 = Base.getfield(rng, :ptr)::Ptr{UInt64}
│   %2 = Base.bitcast(Ptr{UInt8}, %1)::Ptr{UInt8}
│   %3 = VectorizationBase.llvmcall(("    \n\n    define void @entry(i64, i8, i64) alwaysinline {\n    top:\n        %ptr.0 = inttoptr i64 %0 to i8*\n%ptr.1 = getelementptr inbounds i8, i8* %ptr.0, i64 %2\nstore i8 %1, i8* %ptr.1, align 1\nret void\n    }\n", "entry"), VectorizationBase.Cvoid, Tuple{Ptr{UInt8}, UInt8, Int64}, %2, v, 130)::Nothing
└──      return %3
) => Nothing

julia> @code_llvm VectorizedRNG.setrand64counter!(local_rng(), 0x01)
;  @ /Users/dilum/.julia/packages/VectorizedRNG/RVAIZ/src/xoshiro.jl:97 within `setrand64counter!'
define void @"julia_setrand64counter!_583"([1 x i64]* nocapture nonnull readonly align 8 dereferenceable(8) %0, i8 zeroext %1) {
top:
;  @ /Users/dilum/.julia/packages/VectorizedRNG/RVAIZ/src/xoshiro.jl:98 within `setrand64counter!'
; ┌ @ essentials.jl:402 within `unsafe_convert'
; │┌ @ pointer.jl:30 within `convert'
    %2 = bitcast [1 x i64]* %0 to i8**
    %3 = load i8*, i8** %2, align 8
; └└
; ┌ @ /Users/dilum/.julia/packages/VectorizationBase/NMmk1/src/llvm_intrin/memory_addr.jl:782 within `vstore!' @ /Users/dilum/.julia/packages/VectorizationBase/NMmk1/src/llvm_intrin/memory_addr.jl:638
; │┌ @ /Users/dilum/.julia/packages/VectorizationBase/NMmk1/src/llvm_intrin/memory_addr.jl:638 within `macro expansion'
    %ptr.1.i = getelementptr inbounds i8, i8* %3, i64 258
    store i8 %1, i8* %ptr.1.i, align 1
; └└
  ret void
}

julia> @code_llvm VectorizedRNG.setrand64counter!(local_rng(), 0x01)
;  @ /Users/dilum/.julia/packages/VectorizedRNG/RVAIZ/src/xoshiro.jl:97 within `setrand64counter!'
define void @"julia_setrand64counter!_601"([1 x i64]* nocapture nonnull readonly align 8 dereferenceable(8) %0, i8 zeroext %1) {
top:
;  @ /Users/dilum/.julia/packages/VectorizedRNG/RVAIZ/src/xoshiro.jl:98 within `setrand64counter!'
; ┌ @ essentials.jl:402 within `unsafe_convert'
; │┌ @ pointer.jl:30 within `convert'
    %2 = bitcast [1 x i64]* %0 to i8**
    %3 = load i8*, i8** %2, align 8
; └└
; ┌ @ /Users/dilum/.julia/packages/VectorizationBase/NMmk1/src/llvm_intrin/memory_addr.jl:782 within `vstore!' @ /Users/dilum/.julia/packages/VectorizationBase/NMmk1/src/llvm_intrin/memory_addr.jl:638
; │┌ @ /Users/dilum/.julia/packages/VectorizationBase/NMmk1/src/llvm_intrin/memory_addr.jl:638 within `macro expansion'
    %ptr.1.i = getelementptr inbounds i8, i8* %3, i64 258
    store i8 %1, i8* %ptr.1.i, align 1
; └└
  ret void
}

julia> @code_llvm VectorizedRNG.setrand64counter!(local_rng(), 0x01)
;  @ /Users/dilum/.julia/packages/VectorizedRNG/RVAIZ/src/xoshiro.jl:97 within `setrand64counter!'
define void @"julia_setrand64counter!_604"([1 x i64]* nocapture nonnull readonly align 8 dereferenceable(8) %0, i8 zeroext %1) {
top:
;  @ /Users/dilum/.julia/packages/VectorizedRNG/RVAIZ/src/xoshiro.jl:98 within `setrand64counter!'
; ┌ @ essentials.jl:402 within `unsafe_convert'
; │┌ @ pointer.jl:30 within `convert'
    %2 = bitcast [1 x i64]* %0 to i8**
    %3 = load i8*, i8** %2, align 8
; └└
; ┌ @ /Users/dilum/.julia/packages/VectorizationBase/NMmk1/src/llvm_intrin/memory_addr.jl:782 within `vstore!' @ /Users/dilum/.julia/packages/VectorizationBase/NMmk1/src/llvm_intrin/memory_addr.jl:638
; │┌ @ /Users/dilum/.julia/packages/VectorizationBase/NMmk1/src/llvm_intrin/memory_addr.jl:638 within `macro expansion'
    %ptr.1.i = getelementptr inbounds i8, i8* %3, i64 258
    store i8 %1, i8* %ptr.1.i, align 1
; └└
  ret void
}

julia> VectorizationBase.has_feature(::Val{:x86_64_avx2}) = VectorizationBase.False()

julia> @code_llvm VectorizedRNG.setrand64counter!(local_rng(), 0x01)
;  @ /Users/dilum/.julia/packages/VectorizedRNG/RVAIZ/src/xoshiro.jl:97 within `setrand64counter!'
define void @"julia_setrand64counter!_613"([1 x i64]* nocapture nonnull readonly align 8 dereferenceable(8) %0, i8 zeroext %1) {
top:
;  @ /Users/dilum/.julia/packages/VectorizedRNG/RVAIZ/src/xoshiro.jl:98 within `setrand64counter!'
; ┌ @ essentials.jl:402 within `unsafe_convert'
; │┌ @ pointer.jl:30 within `convert'
    %2 = bitcast [1 x i64]* %0 to i8**
    %3 = load i8*, i8** %2, align 8
; └└
; ┌ @ /Users/dilum/.julia/packages/VectorizationBase/NMmk1/src/llvm_intrin/memory_addr.jl:782 within `vstore!' @ /Users/dilum/.julia/packages/VectorizationBase/NMmk1/src/llvm_intrin/memory_addr.jl:638
; │┌ @ /Users/dilum/.julia/packages/VectorizationBase/NMmk1/src/llvm_intrin/memory_addr.jl:638 within `macro expansion'
    %ptr.1.i = getelementptr inbounds i8, i8* %3, i64 130
    store i8 %1, i8* %ptr.1.i, align 1
; └└
  ret void
}

@DilumAluthge
Copy link
Member

However, I cannot reproduce if I start Julia with julia --compiled-modules=no:

dilum:/Users/dilum/Downloads$ julia --compiled-modules=no
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.7.0-DEV.400 (2021-01-28)
 _/ |\__'_|_|_|\__'_|  |  Commit 9de107a63a (5 days old master)
|__/                   |

julia> using VectorizationBase

julia> !success(run(`grep -nr "pure" $(dirname(pathof(VectorizationBase)))`, wait=false))
true

julia> VectorizationBase.has_feature(::Val{:x86_64_avx512f}) = VectorizationBase.False()

julia> VectorizationBase.has_feature(::Val{:x86_64_avx2}) = VectorizationBase.False()

julia> using VectorizedRNG

julia> @code_typed VectorizedRNG.setrand64counter!(local_rng(), 0x01)
CodeInfo(
1 ─ %1 = Base.getfield(rng, :ptr)::Ptr{UInt64}
│   %2 = Base.bitcast(Ptr{UInt8}, %1)::Ptr{UInt8}
│   %3 = VectorizationBase.llvmcall(("    \n\n    define void @entry(i64, i8, i64) alwaysinline {\n    top:\n        %ptr.0 = inttoptr i64 %0 to i8*\n%ptr.1 = getelementptr inbounds i8, i8* %ptr.0, i64 %2\nstore i8 %1, i8* %ptr.1, align 1\nret void\n    }\n", "entry"), VectorizationBase.Cvoid, Tuple{Ptr{UInt8}, UInt8, Int64}, %2, v, 130)::Nothing
└──      return %3
) => Nothing

julia> @code_llvm VectorizedRNG.setrand64counter!(local_rng(), 0x01)
;  @ /Users/dilum/.julia/packages/VectorizedRNG/RVAIZ/src/xoshiro.jl:97 within `setrand64counter!'
define void @"julia_setrand64counter!_1054"([1 x i64]* nocapture nonnull readonly align 8 dereferenceable(8) %0, i8 zeroext %1) {
top:
;  @ /Users/dilum/.julia/packages/VectorizedRNG/RVAIZ/src/xoshiro.jl:98 within `setrand64counter!'
; ┌ @ essentials.jl:402 within `unsafe_convert'
; │┌ @ pointer.jl:30 within `convert'
    %2 = bitcast [1 x i64]* %0 to i8**
    %3 = load i8*, i8** %2, align 8
; └└
; ┌ @ /Users/dilum/.julia/packages/VectorizationBase/NMmk1/src/llvm_intrin/memory_addr.jl:782 within `vstore!' @ /Users/dilum/.julia/packages/VectorizationBase/NMmk1/src/llvm_intrin/memory_addr.jl:638
; │┌ @ /Users/dilum/.julia/packages/VectorizationBase/NMmk1/src/llvm_intrin/memory_addr.jl:638 within `macro expansion'
    %ptr.1.i = getelementptr inbounds i8, i8* %3, i64 130
    store i8 %1, i8* %ptr.1.i, align 1
; └└
  ret void
}

@chriselrod
Copy link
Collaborator Author

Great findings, I can reproduce both that redefining again after loading, or running with --compiled-modules=no, solve the problem.

Seems like precompilation may be connected, where the old stuff isn't invalidated if it hasn't already been loaded?

@chriselrod
Copy link
Collaborator Author

That would explain why setrand64counter! showed the problem, but getrand64counter did not: the former is called in the __init__ function, but the latter isn't.

As an awful hack, I could call the latter too, then at least they'd agree. lol

@DilumAluthge
Copy link
Member

If you can turn this into a script, we can do git bisect run.

@DilumAluthge
Copy link
Member

I.e. instead of just printing @code_typed and @code_llvm, we write a script that extracts the specific number and @tests whether it is 130 or not.

@DilumAluthge
Copy link
Member

Here's the full bisect setup:

script.sh:

#!/bin/bash

rm -rf ~/.julia
make cleanall
make clean
make
rm -rf ~/.julia
./julia install-packages.jl
./julia test.jl

install-packages.jl:

import Pkg 

Pkg.add(Pkg.PackageSpec(name = "ArrayInterface", rev = "impure"))
Pkg.add(Pkg.PackageSpec(name = "VectorizationBase", rev = "master"))
Pkg.add(Pkg.PackageSpec(name = "VectorizedRNG", rev = "master"))

test.jl:

using VectorizationBase

VectorizationBase.has_feature(::Val{:x86_64_avx512f}) = VectorizationBase.False()

VectorizationBase.has_feature(::Val{:x86_64_avx2}) = VectorizationBase.False()

using VectorizedRNG

import InteractiveUtils

typed = string(InteractiveUtils.code_typed(VectorizedRNG.setrand64counter!, Tuple{typeof(local_rng()), typeof(0x01)}))

io = IOBuffer()

InteractiveUtils.code_llvm(io, VectorizedRNG.setrand64counter!, Tuple{typeof(local_rng()), typeof(0x01)})

llvm = String(take!(io))

import Test

Test.@testset begin
    Test.@test occursin("130", typed)
    Test.@test occursin("130", llvm)
    Test.@test !occursin("258", typed)
    Test.@test !occursin("258", llvm)
end

@DilumAluthge
Copy link
Member

Hmmm. For me, I can still reproduce on 1.5.3.

@DilumAluthge
Copy link
Member

DilumAluthge commented Feb 3, 2021

And I can reproduce on 1.5.0:

$ julia
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.5.0 (2020-08-01)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> using VectorizationBase

julia> !success(run(`grep -nr "pure" $(dirname(pathof(VectorizationBase)))`, wait=false))
true

julia> VectorizationBase.has_feature(::Val{:x86_64_avx512f}) = VectorizationBase.False()

julia> VectorizationBase.has_feature(::Val{:x86_64_avx2}) = VectorizationBase.False()

julia> using VectorizedRNG

julia> @code_typed VectorizedRNG.setrand64counter!(local_rng(), 0x01)
CodeInfo(
1 ─ %1 = Base.getfield(rng, :ptr)::Ptr{UInt64}
│   %2 = Base.bitcast(Ptr{UInt8}, %1)::Ptr{UInt8}
│   %3 = VectorizationBase.llvmcall(("", "%ptr.0 = inttoptr i64 %0 to i8*\n%ptr.1 = getelementptr inbounds i8, i8* %ptr.0, i64 %2\nstore i8 %1, i8* %ptr.1, align 1\nret void"), VectorizationBase.Cvoid, Tuple{Ptr{UInt8},UInt8,Int64}, %2, v, 130)::Nothing
└──      return %3
) => Nothing

julia> @code_llvm VectorizedRNG.setrand64counter!(local_rng(), 0x01)

;  @ /Users/dilum/.julia/packages/VectorizedRNG/RVAIZ/src/xoshiro.jl:97 within `setrand64counter!'
define void @"julia_setrand64counter!_1039"([1 x i64]* nocapture nonnull readonly dereferenceable(8), i8) {
top:
;  @ /Users/dilum/.julia/packages/VectorizedRNG/RVAIZ/src/xoshiro.jl:98 within `setrand64counter!'
; ┌ @ essentials.jl:392 within `unsafe_convert'
; │┌ @ pointer.jl:30 within `convert'
    %2 = bitcast [1 x i64]* %0 to i8**
    %3 = load i8*, i8** %2, align 8
; └└
; ┌ @ /Users/dilum/.julia/packages/VectorizationBase/maLbj/src/llvm_intrin/memory_addr.jl:782 within `vstore!' @ /Users/dilum/.julia/packages/VectorizationBase/maLbj/src/llvm_intrin/memory_addr.jl:638
; │┌ @ /Users/dilum/.julia/packages/VectorizationBase/maLbj/src/llvm_intrin/memory_addr.jl:638 within `macro expansion'
    %ptr.1.i = getelementptr inbounds i8, i8* %3, i64 258
    store i8 %1, i8* %ptr.1.i, align 1
; └└
  ret void
}

julia> using ArrayInterface

julia> !success(run(`grep -nr "pure" $(dirname(pathof(ArrayInterface)))`, wait=false))
true

julia> versioninfo()
Julia Version 1.5.0
Commit 96786e22cc (2020-08-01 23:44 UTC)
Platform Info:
  OS: macOS (x86_64-apple-darwin18.7.0)
  CPU: Intel(R) Core(TM) i5-4278U CPU @ 2.60GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-9.0.1 (ORCJIT, haswell)
Environment:
  ASDF_JULIA_VERSION = 1.5.0
  JULIA_NUM_THREADS = auto
  JULIA_PKG_SERVER =
  JULIA_CUDA_VERBOSE = true

@DilumAluthge
Copy link
Member

DilumAluthge commented Feb 3, 2021

On 1.5.0, I can't repro with --compiled-modules=no.

$ julia --compiled-modules=no
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.5.0 (2020-08-01)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> using VectorizationBase

julia> !success(run(`grep -nr "pure" $(dirname(pathof(VectorizationBase)))`, wait=false))
true

julia> VectorizationBase.has_feature(::Val{:x86_64_avx512f}) = VectorizationBase.False()

julia> VectorizationBase.has_feature(::Val{:x86_64_avx2}) = VectorizationBase.False()

julia> using VectorizedRNG

julia> @code_typed VectorizedRNG.setrand64counter!(local_rng(), 0x01)
CodeInfo(
1 ─ %1 = Base.getfield(rng, :ptr)::Ptr{UInt64}
│   %2 = Base.bitcast(Ptr{UInt8}, %1)::Ptr{UInt8}
│   %3 = VectorizationBase.llvmcall(("", "%ptr.0 = inttoptr i64 %0 to i8*\n%ptr.1 = getelementptr inbounds i8, i8* %ptr.0, i64 %2\nstore i8 %1, i8* %ptr.1, align 1\nret void"), VectorizationBase.Cvoid, Tuple{Ptr{UInt8},UInt8,Int64}, %2, v, 130)::Nothing
└──      return %3
) => Nothing

julia> @code_llvm VectorizedRNG.setrand64counter!(local_rng(), 0x01)

;  @ /Users/dilum/.julia/packages/VectorizedRNG/RVAIZ/src/xoshiro.jl:97 within `setrand64counter!'
define void @"julia_setrand64counter!_1664"([1 x i64]* nocapture nonnull readonly dereferenceable(8), i8) {
top:
;  @ /Users/dilum/.julia/packages/VectorizedRNG/RVAIZ/src/xoshiro.jl:98 within `setrand64counter!'
; ┌ @ essentials.jl:392 within `unsafe_convert'
; │┌ @ pointer.jl:30 within `convert'
    %2 = bitcast [1 x i64]* %0 to i8**
    %3 = load i8*, i8** %2, align 8
; └└
; ┌ @ /Users/dilum/.julia/packages/VectorizationBase/maLbj/src/llvm_intrin/memory_addr.jl:782 within `vstore!' @ /Users/dilum/.julia/packages/VectorizationBase/maLbj/src/llvm_intrin/memory_addr.jl:638
; │┌ @ /Users/dilum/.julia/packages/VectorizationBase/maLbj/src/llvm_intrin/memory_addr.jl:638 within `macro expansion'
    %ptr.1.i = getelementptr inbounds i8, i8* %3, i64 130
    store i8 %1, i8* %ptr.1.i, align 1
; └└
  ret void
}

julia> using ArrayInterface

julia> !success(run(`grep -nr "pure" $(dirname(pathof(ArrayInterface)))`, wait=false))
true

julia> versioninfo()
Julia Version 1.5.0
Commit 96786e22cc (2020-08-01 23:44 UTC)
Platform Info:
  OS: macOS (x86_64-apple-darwin18.7.0)
  CPU: Intel(R) Core(TM) i5-4278U CPU @ 2.60GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-9.0.1 (ORCJIT, haswell)
Environment:
  ASDF_JULIA_VERSION = 1.5.0
  JULIA_NUM_THREADS = auto
  JULIA_PKG_SERVER =
  JULIA_CUDA_VERBOSE = true

@chriselrod
Copy link
Collaborator Author

Hmm, I can reproduce on 1.5 too now. Not sure how I wasn't earlier.
I'm working on a minimal reproducer.

@DilumAluthge
Copy link
Member

DilumAluthge commented Feb 3, 2021

Darn. Because we can repro on Julia 1.5.0, we won't be able to bisect.

@DilumAluthge
Copy link
Member

DilumAluthge commented Feb 3, 2021

BTW, you should probably update your comment in the 265 issue to reflect that you can reproduce on 1.5.3.

@Tokazama
Copy link
Member

Tokazama commented Feb 4, 2021

We should probably revisit some of this once aggressive const-prop arrives, but I think it looks good for now.

@chriselrod
Copy link
Collaborator Author

We should probably revisit some of this once aggressive const-prop arrives

We could use if/else definitions to start trying it out on master.

@Tokazama
Copy link
Member

Tokazama commented Feb 4, 2021

We could use if/else definitions to start trying it out on master.

Sounds like a good idea.

As for this PR, if it helps get people on board with solving the original issue that inspired this then it seems like a good idea. There's a possibility that the Symbol related @pure methods need more work but I don't have any code in the registry that depends on it right now.

@chriselrod
Copy link
Collaborator Author

chriselrod commented Feb 4, 2021

Sounds like a good idea.

As for this PR, if it helps get people on board with solving the original issue that inspired this then it seems like a good idea. There's a possibility that the Symbol related @pure methods need more work but I don't have any code in the registry that depends on it right now.

I'm honestly not really sure what to do here. This PR doesn't actually fix the issue, and there's at least a dozen things I'd rather spend my time on than VectorizationBase's relocatability problem at this point (which as of Julia 1.6 should just result in worse performance when it strikes; in Julia 1.5 it could lead to crashes) -- e.g., things that improve performance for everyone ;).

This PR hasn't been thoroughly regression tested, so maybe we should just de-@pure master, where we rely on the constant-prop macro instead?

@Tokazama
Copy link
Member

Tokazama commented Feb 4, 2021

That would actually be my preference b/c I think there are some things here that definitely shouldn't be pure but some I copied from other packages verbatim where I know a lot of other people have approved of them (e.g., tuple_issubset and _sym_to_dims). I might be able to completely replace them, but I won't be able to get into that head space until at least next week.

@chriselrod chriselrod changed the title Remove all @pure, fixes #115. Remove all @pure, does not actually fix #115. Feb 8, 2021
@chriselrod chriselrod closed this Feb 8, 2021
@DilumAluthge DilumAluthge deleted the impure branch February 8, 2021 18:21
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.

3 participants