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

Segfaults on Julia 1.11 #8

Open
KristofferC opened this issue Mar 5, 2024 · 6 comments
Open

Segfaults on Julia 1.11 #8

KristofferC opened this issue Mar 5, 2024 · 6 comments

Comments

@KristofferC
Copy link

This package segfaults on upcoming Julia 1.11 (https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/0520b80_vs_997b49f/VectorizedReduction.primary.log) with

in expression starting at /home/pkgeval/.julia/packages/VectorizedReduction/bsnWJ/test/reduce.jl:4
macro expansion at /home/pkgeval/.julia/packages/VectorizationBase/xE5Tx/src/llvm_intrin/binary_ops.jl:31 [inlined]
vadd_fast at /home/pkgeval/.julia/packages/VectorizationBase/xE5Tx/src/llvm_intrin/binary_ops.jl:31 [inlined]
fmap at /home/pkgeval/.julia/packages/VectorizationBase/xE5Tx/src/vecunroll/fmap.jl:11 [inlined]
vadd_fast at /home/pkgeval/.julia/packages/VectorizationBase/xE5Tx/src/vecunroll/fmap.jl:111 [inlined]
add_fast at /home/pkgeval/.julia/packages/VectorizationBase/xE5Tx/src/base_defs.jl:91 [inlined]
macro expansion at /home/pkgeval/.julia/packages/LoopVectorization/7gWfp/src/reconstruct_loopset.jl:1107 [inlined]
_turbo_! at /home/pkgeval/.julia/packages/LoopVectorization/7gWfp/src/reconstruct_loopset.jl:1107 [inlined]
macro expansion at /home/pkgeval/.julia/packages/VectorizedReduction/bsnWJ/src/vmapreduce.jl:236 [inlined]
vvmapreduce at /home/pkgeval/.julia/packages/VectorizedReduction/bsnWJ/src/vmapreduce.jl:231
vvreduce at /home/pkgeval/.julia/packages/VectorizedReduction/bsnWJ/src/vmapreduce.jl:147
unknown function (ip: 0x7fdeccb92373)
_jl_invoke at /source/src/gf.c:2945 [inlined]
ijl_apply_generic at /source/src/gf.c:3122
jl_apply at /source/src/julia.h:2165 [inlined]
do_call at /source/src/interpreter.c:126

Note that the LoopVectorization + VectorizationBase ecosystem is deprecated on 1.11 (due to issues like this) so it might be best to avoid them for now

@andrewjradcliffe
Copy link
Owner

No choice but to deprecate this package on 1.11, as it is little more than codegen piped to LoopVectorization. I'll update the README and semver constraints accordingly.

@schrimpf
Copy link

schrimpf commented May 1, 2024

I believe issue here is that when reducing over an empty collection, code like the following is generated:

A = Float64[]
out = zero(eltype(A))
@turbo for i in eachindex(A)
    out += A[i]
end

LoopVectorization warns users not iterate over an empty collection. Changing to @turbo check_empty=true for i in eachindex(A) should be a quick fix. Admittedly, this is only worthwhile if LoopVectorization does not get deprecated.

@andrewjradcliffe
Copy link
Owner

Thanks for the debugging, Paul. That is indeed the general form of the cause of the segfaults on Julia 1.11. It was not much more effort than a find and replace (easy with projectile), so I went ahead and applied it.

LoopVectorization is the major source of performance, but the code generation for involving multiple multidimensional arrays is identical to the optimal loop structure one would write by hand, hence, it can still be quite convenient to use the functions. However, the interface leaves something to be desired (vv, vt prefixes are verbose and don't clearly map to Base versions). I plan to reassess interest once 1.11 is the stable, and if there is interest, create a clean interface. Interfaces aside, an obvious improvement is to switch to Base.Threads for the multi-threaded loops, since Polyester will also be deprecated.

@chriselrod
Copy link

chriselrod commented May 7, 2024

FWIW, all of LoopVectorization's tests pass on 1.11 beta, so I wouldn't bother transitioning off until you have a reason. https://github.com/JuliaSIMD/LoopVectorization.jl/actions/runs/8946228352

Maybe check_empty=true should be the default.

@andrewjradcliffe
Copy link
Owner

Maybe check_empty=true should be the default.

It is now the default in the latest release of this package. I should have set it as the default 2 years ago -_-;;

all of LoopVectorization's tests pass on 1.11 beta

I noticed that. Furthermore, the test suite for this package (which forces LoopVectorization to compile a wide variety of code) passes on Julia master branch.

so I wouldn't bother transitioning off until you have a reason.

Completely agree.

@chriselrod
Copy link

I noticed that. Furthermore, the test suite for this package (which forces LoopVectorization to compile a wide variety of code) passes on Julia master branch.

LoopVectorization's own test failures on master are from checking that nothing printed to stderr:
https://github.com/JuliaSIMD/LoopVectorization.jl/actions/runs/8946228355/job/24576518019
We suddenly get a lot of warnings printing about JuliaLang/julia#53687
I think it'd also be good to switch the llvmcalls that LV uses to opaque pointers at the same time, as opaque pointer mode becomes the default in LLVM 17 and will eventually be dropped.

@andrewjradcliffe andrewjradcliffe pinned this issue May 17, 2024
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

No branches or pull requests

4 participants