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

Deprecate on 1.11 #523

Closed
wants to merge 5 commits into from
Closed

Deprecate on 1.11 #523

wants to merge 5 commits into from

Conversation

maleadt
Copy link

@maleadt maleadt commented Jan 6, 2024

Alternative for a4a160f, only disabling the macros but keeping all other functionality.

Fixes #520, re-fixes #518

@staticfloat
Copy link

I suggest bumping the minor version number (since this is prerelease) as well

@maleadt
Copy link
Author

maleadt commented Jan 6, 2024

That would be a breaking release, and require the whole ecosystem to go through a round of CompatHelper before PkgEval recovers (defeating most of the point of doing this).

Seeing how this actively segfaults on 1.11 in several packages, I don't see why we should keep users from getting this fix automatically, especially if it doesn't affect semantics.

EDIT: it asserts, doesn't segfault, but I don't think we can guarantee that the generate code does the right thing given the failed assertion.

@maleadt
Copy link
Author

maleadt commented Jan 9, 2024

@chriselrod Can you approve this PR for CI to run, and if it passes, merge + tag?

Copy link

codecov bot commented Jan 9, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (3fbe248) 80.31% compared to head (2cbecf4) 80.37%.

Files Patch % Lines
src/constructors.jl 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #523      +/-   ##
==========================================
+ Coverage   80.31%   80.37%   +0.06%     
==========================================
  Files          39       39              
  Lines        9604     9602       -2     
==========================================
+ Hits         7713     7718       +5     
+ Misses       1891     1884       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@maleadt
Copy link
Author

maleadt commented Jan 10, 2024

Well, this won't work either... See the report here: https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/2cbecf4_vs_18b4f3f/report.html

AFAICT, it turns out @turbo is semantically meaningful and can't just be replaced with @inbounds @fastmath. See for example this failure introduced by this PR:

  MethodError: no method matching getindex(::LayoutPointers.StridedPointer{Float64, 2, 2, 0, (2, 1), Tuple{Int64, Static.StaticInt{8}}, Tuple{Static.StaticInt{0}, Static.StaticInt{0}}}, ::Int64, ::Int64)
  Stacktrace:
    [1] macro expansion
      @ TriangularSolve ~/.julia/packages/TriangularSolve/nmXGO/src/TriangularSolve.jl:531 [inlined]
    [2] macro expansion
      @ TriangularSolve ~/.julia/packages/LoopVectorization/r7IqM/src/constructors.jl:407 [inlined]
    [3] nmuladd!(C::LayoutPointers.StridedPointer{Float64, 2, 2, 0, (2, 1), Tuple{Int64, Static.StaticInt{8}}, Tuple{Static.StaticInt{0}, Static.StaticInt{0}}}, A::LayoutPointers.StridedPointer{Float64, 2, 2, 0, (2, 1), Tuple{Int64, Static.StaticInt{8}}, Tuple{Static.StaticInt{0}, Static.StaticInt{0}}}, U::LayoutPointers.StridedPointer{Float64, 2, 2, 0, (2, 1), Tuple{Int64, Static.StaticInt{8}}, Tuple{Static.StaticInt{0}, Static.StaticInt{0}}}, M::Int64, K::Int64, N::Int64)
      @ TriangularSolve ~/.julia/packages/TriangularSolve/nmXGO/src/TriangularSolve.jl:530
    [4] rdiv_block_N!(spc::LayoutPointers.StridedPointer{Float64, 2, 2, 0, (2, 1), Tuple{Int64, Static.StaticInt{8}}, Tuple{Static.StaticInt{0}, Static.StaticInt{0}}}, spa::LayoutPointers.StridedPointer{Float64, 2, 2, 0, (2, 1), Tuple{Int64, Static.StaticInt{8}}, Tuple{Static.StaticInt{0}, Static.StaticInt{0}}}, spu::LayoutPointers.StridedPointer{Float64, 2, 2, 0, (2, 1), Tuple{Int64, Static.StaticInt{8}}, Tuple{Static.StaticInt{0}, Static.StaticInt{0}}}, M::Int64, N::Int64, ::Val{true}, ::Static.StaticInt{2}, Bsize::Int64)
      @ TriangularSolve ~/.julia/packages/TriangularSolve/nmXGO/src/TriangularSolve.jl:578
    [5] rdiv_block_MandN!(spc::LayoutPointers.StridedPointer{Float64, 2, 2, 0, (2, 1), Tuple{Int64, Static.StaticInt{8}}, Tuple{Static.StaticInt{0}, Static.StaticInt{0}}}, spa::LayoutPointers.StridedPointer{Float64, 2, 2, 0, (2, 1), Tuple{Int64, Static.StaticInt{8}}, Tuple{Static.StaticInt{0}, Static.StaticInt{0}}}, spu::LayoutPointers.StridedPointer{Float64, 2, 2, 0, (2, 1), Tuple{Int64, Static.StaticInt{8}}, Tuple{Static.StaticInt{0}, Static.StaticInt{0}}}, M::Int64, N::Int64, ::Val{true}, ::Static.StaticInt{2})
      @ TriangularSolve ~/.julia/packages/TriangularSolve/nmXGO/src/TriangularSolve.jl:599
    [6] div_dispatch!(C::StrideArraysCore.PtrArray{Float64, 2, (2, 1), Tuple{Int64, Int64}, Tuple{StrideArraysCore.StrideReset{Int64}, Nothing}, Tuple{Static.StaticInt{1}, Static.StaticInt{1}}}, A::StrideArraysCore.PtrArray{Float64, 2, (2, 1), Tuple{Int64, Int64}, Tuple{StrideArraysCore.StrideReset{Int64}, Nothing}, Tuple{Static.StaticInt{1}, Static.StaticInt{1}}}, U::StrideArraysCore.PtrArray{Float64, 2, (2, 1), Tuple{Int64, Int64}, Tuple{StrideArraysCore.StrideReset{Int64}, Nothing}, Tuple{Static.StaticInt{1}, Static.StaticInt{1}}}, nthread::Int64, ::Val{true})
      @ TriangularSolve ~/.julia/packages/TriangularSolve/nmXGO/src/TriangularSolve.jl:310 [inlined]
    [7] ldiv!
      @ ~/.julia/packages/TriangularSolve/nmXGO/src/TriangularSolve.jl:479 [inlined]
    [8] reckernel!(A::StrideArraysCore.PtrArray{Float64, 2, (1, 2), Tuple{Int64, Int64}, Tuple{Nothing, StrideArraysCore.StrideReset{Int64}}, Tuple{Static.StaticInt{1}, Static.StaticInt{1}}}, pivot::Val{true}, m::Int64, n::Int64, ipiv::StrideArraysCore.PtrArray{Int64, 1, (1,), Tuple{Int64}, Tuple{Nothing}, Tuple{Static.StaticInt{1}}}, info::Int64, blocksize::Int64, thread::Val{false})
      @ RecursiveFactorization ~/.julia/packages/RecursiveFactorization/cDP6H/src/lu.jl:223

So packages are invoking @turbo loops with StridedPointer inputs, which only LoopVectorization.jl knows how to handle.

At least the report also shows that LoopVectorization.jl is the source of 6 packages segfaulting, so getting it fixed (or successfully deprecated) for 1.11 is important.

@chriselrod
Copy link
Member

Regarding the change in semantics,

  1. TriangularSolve.jl is my package, so I could update it as needed.
  2. It's useless without LV, anyway, so would more or less be deprecated with it.

I doubt anyone other than me used StridedPointers instead of arrays.

@brenhinkeller
Copy link
Contributor

I wonder if it might be a stopgap alternative to just deprecate on Arrays for now

@chriselrod
Copy link
Member

I wonder if it might be a stopgap alternative to just deprecate on Arrays for now

Not much code would be hitting this case. The only way that comes to mind is when passing an array to a function as an argument, without indexing the array as far as the macro can "see".

@chriselrod chriselrod force-pushed the main branch 4 times, most recently from 7e86622 to eeaa0b2 Compare May 3, 2024 23:53
@chriselrod
Copy link
Member

Fixed in a3e8081

@chriselrod chriselrod closed this Jul 31, 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
5 participants