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.

Sign up for GitHub

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

Jump to bottom

Optimize mat_entry_ptr methods #1871

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+28 −30
Open

Optimize mat_entry_ptr methods #1871

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions src/flint/FlintTypes.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4696,10 +4696,10 @@ const QQMatrixSpace = AbstractAlgebra.Generic.MatSpace{QQFieldElem}
QQMatrixSpace(r::Int, c::Int) = QQMatrixSpace(QQ, r, c)

mutable struct QQMatrix <: MatElem{QQFieldElem}
entries::Ptr{Nothing}
entries::Ptr{QQFieldElem}
r::Int
c::Int
rows::Ptr{Nothing}
rows::Ptr{Ptr{QQFieldElem}}
view_parent

# used by windows, not finalised!!
Expand Down Expand Up @@ -4912,10 +4912,10 @@ end
const zzModMatrixSpace = AbstractAlgebra.Generic.MatSpace{zzModRingElem}

mutable struct zzModMatrix <: MatElem{zzModRingElem}
entries::Ptr{Nothing}
entries::Ptr{UInt}
r::Int # Int
c::Int # Int
rows::Ptr{Nothing}
rows::Ptr{Ptr{UInt}}
n::UInt # mp_limb_t / Culong
ninv::UInt # mp_limb_t / Culong
norm::UInt # mp_limb_t / Culong
Expand Down Expand Up @@ -5057,10 +5057,10 @@ end
const ZZModMatrixSpace = AbstractAlgebra.Generic.MatSpace{ZZModRingElem}

mutable struct ZZModMatrix <: MatElem{ZZModRingElem}
entries::Ptr{Nothing}
entries::Ptr{ZZRingElem}
r::Int
c::Int
rows::Ptr{Nothing}
rows::Ptr{Ptr{ZZRingElem}}
# end flint struct

base_ring::ZZModRing
Expand Down Expand Up @@ -5215,10 +5215,10 @@ end
const FpMatrixSpace = AbstractAlgebra.Generic.MatSpace{FpFieldElem}

mutable struct FpMatrix <: MatElem{FpFieldElem}
entries::Ptr{Nothing}
entries::Ptr{ZZRingElem}
r::Int
c::Int
rows::Ptr{Nothing}
rows::Ptr{Ptr{ZZRingElem}}
# end flint struct

base_ring::FpField
Expand Down Expand Up @@ -5324,10 +5324,10 @@ end
const fpMatrixSpace = AbstractAlgebra.Generic.MatSpace{fpFieldElem}

mutable struct fpMatrix <: MatElem{fpFieldElem}
entries::Ptr{Nothing}
entries::Ptr{UInt}
r::Int # Int
c::Int # Int
rows::Ptr{Nothing}
rows::Ptr{Ptr{UInt}}
n::UInt # mp_limb_t / Culong
ninv::UInt # mp_limb_t / Culong
norm::UInt # mp_limb_t / Culong
Expand Down Expand Up @@ -6071,10 +6071,10 @@ end
const FqPolyRepMatrixSpace = AbstractAlgebra.Generic.MatSpace{FqPolyRepFieldElem}

mutable struct FqPolyRepMatrix <: MatElem{FqPolyRepFieldElem}
entries::Ptr{Nothing}
entries::Ptr{FqPolyRepFieldElem}
r::Int
c::Int
rows::Ptr{Nothing}
rows::Ptr{Ptr{FqPolyRepFieldElem}}
base_ring::FqPolyRepField
view_parent

Expand Down Expand Up @@ -6204,10 +6204,10 @@ end
const fqPolyRepMatrixSpace = AbstractAlgebra.Generic.MatSpace{fqPolyRepFieldElem}

mutable struct fqPolyRepMatrix <: MatElem{fqPolyRepFieldElem}
entries::Ptr{Nothing}
entries::Ptr{fqPolyRepFieldElem}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is not 100% true, since otherwise I would expect the following to work (before this PR, with htis PR the Ptr conversion becomes unnecessary):

julia> F = Native.GF(2,2);

julia> a = matrix(F, 2, 2, [1,2,3,4]);

julia> unsafe_load(Ptr{fqPolyRepFieldElem}(a.entries))

[1054580] signal (11.1): Segmentation fault

Copy link
Member Author

Choose a reason for hiding this comment

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

It definitely is a lie, just like with many other Ptr{ZZRingElem} etc. etc. in our code base -- our Julia types are "supersets" of the FLINT types.

BTW you can get the same crash in master via unsafe_load(Nemo.mat_entry_ptr(a, 1, 1)). I simply chose the signatures here to make the implementation of mat_entry_ptr as simple as possible for each type.

We could of course also resolve this by adding a separate struct fq_nmod_t (and fmpz_t etc. etc.) which faithfully replicate the FLINT structs, nothing more and nothing less. Then e.g. fqPolyRepFieldElem could start with a member x:: fq_nmod_t.

That way, we could resolve all the lies everywhere. But that would be a major piece of work for relatively limited gain (if I were to start Nemo today, that's more or less what I'd do anyway, but as it is now...).

Anyway: I am happy to discuss this further, but I think for the purpose of this PR, it doesn't really make anything worse, as nothing in Nemo.jl ever accesses .entries fields, and the access to .rows is limited to very, very few places. And to me at least it was helpful to get a better idea of what each entries and rows points to. But I am also ok with changing them back to Nothing and adjust the mat_entry_ptr to insert suitable reinterpret calls.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the explanation. I am fine with merging this. Could you open an issue with basically what you explained here, so that we keep it somewhere? (even though that's nothing to be changed in the near future (unless someone has way too much time))

r::Int
c::Int
rows::Ptr{Nothing}
rows::Ptr{Ptr{fqPolyRepFieldElem}}
base_ring::fqPolyRepField
view_parent

Expand Down
4 changes: 1 addition & 3 deletions src/flint/fmpq_mat.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1010,9 +1010,7 @@ end
#
################################################################################

@inline mat_entry_ptr(A::QQMatrix, i::Int, j::Int) =
ccall((:fmpq_mat_entry, libflint),
Ptr{QQFieldElem}, (Ref{QQMatrix}, Int, Int), A, i-1, j-1)
mat_entry_ptr(A::QQMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*2*sizeof(Ptr)
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
mat_entry_ptr(A::QQMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*2*sizeof(Ptr)
mat_entry_ptr(A::QQMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(QQFieldElem)


################################################################################
#
Expand Down
2 changes: 1 addition & 1 deletion src/flint/fmpz_mat.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2107,4 +2107,4 @@ end
#
################################################################################

@inline mat_entry_ptr(A::ZZMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(UInt)
mat_entry_ptr(A::ZZMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(Ptr)
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
mat_entry_ptr(A::ZZMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(Ptr)
mat_entry_ptr(A::ZZMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(ZZRingElem)

4 changes: 1 addition & 3 deletions src/flint/fmpz_mod_mat.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -900,6 +900,4 @@ end
#
################################################################################

@inline mat_entry_ptr(A::ZZModMatrix, i::Int, j::Int) =
ccall((:fmpz_mod_mat_entry, libflint), Ptr{ZZRingElem},
(Ref{ZZModMatrix}, Int, Int), A, i - 1, j - 1)
mat_entry_ptr(A::ZZModMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(Ptr)
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
mat_entry_ptr(A::ZZModMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(Ptr)
mat_entry_ptr(A::ZZModMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(ZZRingElem)

4 changes: 1 addition & 3 deletions src/flint/fq_mat.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -764,6 +764,4 @@ end
#
################################################################################

@inline mat_entry_ptr(A::FqPolyRepMatrix, i::Int, j::Int) =
ccall((:fq_mat_entry, libflint), Ptr{FqPolyRepFieldElem},
(Ref{FqPolyRepMatrix}, Int, Int), A, i - 1, j - 1)
mat_entry_ptr(A::FqPolyRepMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*(sizeof(Ptr)+2*sizeof(Int))
Copy link
Member Author

@fingolfin fingolfin Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is a bit confusing, perhaps I should add a comment

Suggested change
mat_entry_ptr(A::FqPolyRepMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*(sizeof(Ptr)+2*sizeof(Int))
# each matrix entry consists of
# coeffs :: Ptr{Nothing}
# alloc :: Int
# length :: Int
# The `parent` member of struct FqPolyRepFieldElem is not replicated in each
# struct member, so we cannot use `sizeof(FqPolyRepFieldElem)`.
mat_entry_ptr(A::FqPolyRepMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*(sizeof(Ptr)+2*sizeof(Int))

4 changes: 1 addition & 3 deletions src/flint/fq_nmod_mat.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,6 +753,4 @@ end
#
################################################################################

@inline mat_entry_ptr(A::fqPolyRepMatrix, i::Int, j::Int) =
ccall((:fq_nmod_mat_entry, libflint), Ptr{fqPolyRepFieldElem},
(Ref{fqPolyRepMatrix}, Int, Int), A, i - 1, j - 1)
mat_entry_ptr(A::fqPolyRepMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*(sizeof(Ptr)+5*sizeof(Int))
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
mat_entry_ptr(A::fqPolyRepMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*(sizeof(Ptr)+5*sizeof(Int))
# each matrix entry consists of
# coeffs :: Ptr{Nothing}
# alloc :: Int
# length :: Int
# n :: Int
# ninv :: Int
# norm :: Int
# The `parent` member of struct fqPolyRepFieldElem is not replicated in each
# struct member, so we cannot simply use `sizeof(fqPolyRepFieldElem)`.
mat_entry_ptr(A::fqPolyRepMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*(sizeof(Ptr)+5*sizeof(Int))

4 changes: 1 addition & 3 deletions src/flint/gfp_fmpz_mat.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,4 @@ end
#
################################################################################

@inline mat_entry_ptr(A::FpMatrix, i::Int, j::Int) =
ccall((:fmpz_mod_mat_entry, libflint), Ptr{ZZRingElem},
(Ref{FpMatrix}, Int, Int), A, i - 1, j - 1)
mat_entry_ptr(A::FpMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(Ptr)
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
mat_entry_ptr(A::FpMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(Ptr)
mat_entry_ptr(A::FpMatrix, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(ZZRingElem)

8 changes: 8 additions & 0 deletions src/flint/nmod_mat.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -898,3 +898,11 @@
end

# For kernel(::HowellFormTrait, ...) we use generic AbstractAlgebra code

################################################################################
#
# Entry pointers
#
################################################################################

mat_entry_ptr(A::Zmodn_mat, i::Int, j::Int) = unsafe_load(A.rows, i) + (j-1)*sizeof(UInt)

Check warning on line 908 in src/flint/nmod_mat.jl

View check run for this annotation

Codecov / codecov/patch

src/flint/nmod_mat.jl#L908 

Added line #L908 was not covered by tests
Loading