Skip to content

Commit

Permalink
Fix 32 bit issue with cholmod.jl
Browse files Browse the repository at this point in the history
  • Loading branch information
@andreasnoack
andreasnoack committed Feb 16, 2015
1 parent 39516b6 commit df2eff3
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 33 deletions.
36 changes: 18 additions & 18 deletions base/sparse/cholmod.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ using Base.SparseMatrix: AbstractSparseMatrix, SparseMatrixCSC, increment, indty
include("cholmod_h.jl")

## macro to generate the name of the C function according to the integer type
macro cholmod_name(nm,typ) string("cholmod_", eval(typ) == Int64 ? "l_" : "", nm) end
macro cholmod_name(nm,typ) string("cholmod_", eval(typ) == Int ? "l_" : "", nm) end

for Ti in IndexTypes
@eval begin
Expand Down Expand Up @@ -198,54 +198,54 @@ end

### cholmod_core_h ###
function allocate_dense(nrow::Integer, ncol::Integer, d::Integer, ::Type{Float64})
d = Dense(ccall((:cholmod_allocate_dense, :libcholmod), Ptr{C_Dense{Float64}},
d = Dense(ccall((:cholmod_l_allocate_dense, :libcholmod), Ptr{C_Dense{Float64}},
(Csize_t, Csize_t, Csize_t, Cint, Ptr{Void}),
nrow, ncol, d, REAL, common(Cint)))
nrow, ncol, d, REAL, common(Int)))
finalizer(d, free!)
d
end
function allocate_dense(nrow::Integer, ncol::Integer, d::Integer, ::Type{Complex{Float64}})
d = Dense(ccall((:cholmod_allocate_dense, :libcholmod), Ptr{C_Dense{Complex{Float64}}},
d = Dense(ccall((:cholmod_l_allocate_dense, :libcholmod), Ptr{C_Dense{Complex{Float64}}},
(Csize_t, Csize_t, Csize_t, Cint, Ptr{Void}),
nrow, ncol, d, COMPLEX, common(Cint)))
nrow, ncol, d, COMPLEX, common(Int)))
finalizer(d, free!)
d
end

free_dense!{T}(p::Ptr{C_Dense{T}}) = ccall((:cholmod_free_dense, :libcholmod), Cint, (Ptr{Ptr{C_Dense{T}}}, Ptr{Void}), &p, common(Cint))
free_dense!{T}(p::Ptr{C_Dense{T}}) = ccall((:cholmod_l_free_dense, :libcholmod), Cint, (Ptr{Ptr{C_Dense{T}}}, Ptr{Void}), &p, common(Cint))

function zeros{T<:VTypes}(m::Integer, n::Integer, ::Type{T})
d = Dense(ccall((:cholmod_zeros, :libcholmod), Ptr{C_Dense{T}},
d = Dense(ccall((:cholmod_l_zeros, :libcholmod), Ptr{C_Dense{T}},
(Csize_t, Csize_t, Cint, Ptr{UInt8}),
m, n, xtyp(T), common(Cint)))
m, n, xtyp(T), common(Int)))
finalizer(d, free!)
d
end
zeros(m::Integer, n::Integer) = zeros(m, n, Float64)

function ones{T<:VTypes}(m::Integer, n::Integer, ::Type{T})
d = Dense(ccall((:cholmod_ones, :libcholmod), Ptr{C_Dense{T}},
d = Dense(ccall((:cholmod_l_ones, :libcholmod), Ptr{C_Dense{T}},
(Csize_t, Csize_t, Cint, Ptr{UInt8}),
m, n, xtyp(T), common(Cint)))
m, n, xtyp(T), common(Int)))
finalizer(d, free!)
d
end
ones(m::Integer, n::Integer) = ones(m, n, Float64)

function eye{T<:VTypes}(m::Integer, n::Integer, ::Type{T})
d = Dense(ccall((:cholmod_eye, :libcholmod), Ptr{C_Dense{T}},
d = Dense(ccall((:cholmod_l_eye, :libcholmod), Ptr{C_Dense{T}},
(Csize_t, Csize_t, Cint, Ptr{UInt8}),
m, n, xtyp(T), common(Cint)))
m, n, xtyp(T), common(Int)))
finalizer(d, free!)
d
end
eye(m::Integer, n::Integer) = eye(m, n, Float64)
eye(n::Integer) = eye(n, n, Float64)

function copy_dense{Tv<:VTypes}(A::Dense{Tv})
d = Dense(ccall((:cholmod_copy_dense, :libcholmod), Ptr{C_Dense{Tv}},
d = Dense(ccall((:cholmod_l_copy_dense, :libcholmod), Ptr{C_Dense{Tv}},
(Ptr{C_Dense{Tv}}, Ptr{UInt8}),
A.p, common(Cint)))
A.p, common(Int)))
finalizer(d, free!)
d
end
Expand All @@ -260,16 +260,16 @@ function norm_dense{Tv<:VTypes}(D::Dense{Tv}, p::Integer)
elseif p != 0 && p != 1
throw(ArgumentError("second argument must be either 0 (Inf norm), 1, or 2"))
end
ccall((:cholmod_norm_dense, :libcholmod), Cdouble,
ccall((:cholmod_l_norm_dense, :libcholmod), Cdouble,
(Ptr{C_Dense{Tv}}, Cint, Ptr{UInt8}),
D.p, p, common(Cint))
D.p, p, common(Int))
end

### cholmod_check.h ###
function check_dense{T<:VTypes}(A::Dense{T})
bool(ccall((:cholmod_check_dense, :libcholmod), Cint,
bool(ccall((:cholmod_l_check_dense, :libcholmod), Cint,
(Ptr{C_Dense{T}}, Ptr{UInt8}),
A.p, common(Cint)))
A.p, common(Int)))
end

# Non-Dense wrappers (which all depend on IType)
Expand Down
30 changes: 15 additions & 15 deletions test/sparsedir/cholmod.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,51 +194,51 @@ run(`rm tmp.mtx`)
# test that Sparse(Ptr) constructor throws the right places
## The struct pointer must be constructed by the library constructor and then modified afterwards to checks that the method throws
### illegal dtype (for now but should be supprted at some point)
p = ccall((:cholmod_allocate_sparse, :libcholmod), Ptr{CHOLMOD.C_SparseVoid},
p = ccall((:cholmod_l_allocate_sparse, :libcholmod), Ptr{CHOLMOD.C_SparseVoid},
(Csize_t, Csize_t, Csize_t, Cint, Cint, Cint, Cint, Ptr{Void}),
1, 1, 1, true, true, 0, CHOLMOD.REAL, CHOLMOD.common(Cint))
1, 1, 1, true, true, 0, CHOLMOD.REAL, CHOLMOD.common(Int))
puint = convert(Ptr{Uint32}, p)
unsafe_store!(puint, CHOLMOD.SINGLE, 3*div(sizeof(Csize_t), 4) + 5*div(sizeof(Ptr{Void}), 4) + 4)
@test_throws CHOLMOD.CHOLMODException CHOLMOD.Sparse(p)

### illegal dtype
p = ccall((:cholmod_allocate_sparse, :libcholmod), Ptr{CHOLMOD.C_SparseVoid},
p = ccall((:cholmod_l_allocate_sparse, :libcholmod), Ptr{CHOLMOD.C_SparseVoid},
(Csize_t, Csize_t, Csize_t, Cint, Cint, Cint, Cint, Ptr{Void}),
1, 1, 1, true, true, 0, CHOLMOD.REAL, CHOLMOD.common(Cint))
1, 1, 1, true, true, 0, CHOLMOD.REAL, CHOLMOD.common(Int))
puint = convert(Ptr{Uint32}, p)
unsafe_store!(puint, 5, 3*div(sizeof(Csize_t), 4) + 5*div(sizeof(Ptr{Void}), 4) + 4)
@test_throws CHOLMOD.CHOLMODException CHOLMOD.Sparse(p)

### illegal xtype
p = ccall((:cholmod_allocate_sparse, :libcholmod), Ptr{CHOLMOD.C_SparseVoid},
p = ccall((:cholmod_l_allocate_sparse, :libcholmod), Ptr{CHOLMOD.C_SparseVoid},
(Csize_t, Csize_t, Csize_t, Cint, Cint, Cint, Cint, Ptr{Void}),
1, 1, 1, true, true, 0, CHOLMOD.REAL, CHOLMOD.common(Cint))
1, 1, 1, true, true, 0, CHOLMOD.REAL, CHOLMOD.common(Int))
puint = convert(Ptr{Uint32}, p)
unsafe_store!(puint, 3, 3*div(sizeof(Csize_t), 4) + 5*div(sizeof(Ptr{Void}), 4) + 3)
@test_throws CHOLMOD.CHOLMODException CHOLMOD.Sparse(p)

### illegal itype
p = ccall((:cholmod_allocate_sparse, :libcholmod), Ptr{CHOLMOD.C_SparseVoid},
p = ccall((:cholmod_l_allocate_sparse, :libcholmod), Ptr{CHOLMOD.C_SparseVoid},
(Csize_t, Csize_t, Csize_t, Cint, Cint, Cint, Cint, Ptr{Void}),
1, 1, 1, true, true, 0, CHOLMOD.REAL, CHOLMOD.common(Cint))
1, 1, 1, true, true, 0, CHOLMOD.REAL, CHOLMOD.common(Int))
puint = convert(Ptr{Uint32}, p)
unsafe_store!(puint, CHOLMOD.INTLONG, 3*div(sizeof(Csize_t), 4) + 5*div(sizeof(Ptr{Void}), 4) + 2)
@test_throws CHOLMOD.CHOLMODException CHOLMOD.Sparse(p)

### illegal itype
p = ccall((:cholmod_allocate_sparse, :libcholmod), Ptr{CHOLMOD.C_SparseVoid},
p = ccall((:cholmod_l_allocate_sparse, :libcholmod), Ptr{CHOLMOD.C_SparseVoid},
(Csize_t, Csize_t, Csize_t, Cint, Cint, Cint, Cint, Ptr{Void}),
1, 1, 1, true, true, 0, CHOLMOD.REAL, CHOLMOD.common(Cint))
1, 1, 1, true, true, 0, CHOLMOD.REAL, CHOLMOD.common(Int))
puint = convert(Ptr{Uint32}, p)
unsafe_store!(puint, 5, 3*div(sizeof(Csize_t), 4) + 5*div(sizeof(Ptr{Void}), 4) + 2)
@test_throws CHOLMOD.CHOLMODException CHOLMOD.Sparse(p)

# test that Sparse(Ptr) works for SuiteSparse_long (on 64 bit systems)
if CHOLMOD.SuiteSparse_long == Int64
p = ccall((:cholmod_l_allocate_sparse, :libcholmod), Ptr{CHOLMOD.C_SparseVoid},
p = ccall((:cholmod_allocate_sparse, :libcholmod), Ptr{CHOLMOD.C_SparseVoid},
(Csize_t, Csize_t, Csize_t, Cint, Cint, Cint, Cint, Ptr{Void}),
1, 1, 1, true, true, 0, CHOLMOD.REAL, CHOLMOD.common(CHOLMOD.SuiteSparse_long))
@test isa(CHOLMOD.Sparse(p), CHOLMOD.Sparse{Float64,CHOLMOD.SuiteSparse_long})
1, 1, 1, true, true, 0, CHOLMOD.REAL, CHOLMOD.common(Int32))
@test isa(CHOLMOD.Sparse(p), CHOLMOD.Sparse{Float64,Int32})
end

# Test Dense wrappers (only Float64 supported a present)
Expand Down Expand Up @@ -283,9 +283,9 @@ end

# Test Sparse and Factor
## test free_sparse!
p = ccall((:cholmod_allocate_sparse, :libcholmod), Ptr{CHOLMOD.C_Sparse{Float64,Cint}},
p = ccall((:cholmod_l_allocate_sparse, :libcholmod), Ptr{CHOLMOD.C_Sparse{Float64,Cint}},
(Csize_t, Csize_t, Csize_t, Cint, Cint, Cint, Cint, Ptr{Void}),
1, 1, 1, true, true, 0, CHOLMOD.REAL, CHOLMOD.common(Cint))
1, 1, 1, true, true, 0, CHOLMOD.REAL, CHOLMOD.common(Int))
@test CHOLMOD.free_sparse!(p)

for elty in (Float64, Complex{Float64})
Expand Down

7 comments on commit df2eff3

@tkelman
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks completely backwards

@andreasnoack
Copy link
Member Author

Choose a reason for hiding this comment

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

The SuiteSparse_long is similar to our Int, i.e. determined by the architecture. On 64 bit, CHOLMOD and UMFPACK let you use Cint/Int32 for the integers, but this option is not there for SPQR which call the _l_ versions of the functions. On 32 bit there is really no difference between the _l_ and the non-_l_ version, but they cannot be mixed so therefore I had to change our default such that we now call the _l_ versions whenever the chosen integer type is the same as Int. Does it make sense?

@tkelman
Copy link
Contributor

Choose a reason for hiding this comment

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

I think so, but please include this kind of information in the commit message, comments, or both.

The SuiteSparse_long is similar to our Int, i.e. determined by the architecture.

Mostly. It's set to long on all architectures except for Win64, where it's set to __int64. I don't believe we're overwriting this behavior anywhere.

@andreasnoack
Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't our Int always a 32 bit integer type on 32 bit systems and a 64 bit integer type on 64 bit systems just like SuiteSparse_long? I believe I'm relying on this assumption in the cholmod_name macro.

@tkelman
Copy link
Contributor

Choose a reason for hiding this comment

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

With the default configuration of suitesparse, yes. However when we're using system versions of this library we're at the mercy of how suitesparse was configured. I'd prefer we make as few assumptions as possible, and rely on the information that we actually get from the library (our wrapper, anyway) about how it was configured.

@andreasnoack
Copy link
Member Author

Choose a reason for hiding this comment

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

Okay. Thanks for the comments. I've changed the dependence on Int to SuiteSparse_long which is detected from the linked library. I'll push as soon as the tests pass locally.

@ViralBShah
Copy link
Member

Choose a reason for hiding this comment

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

This is certainly a better way to go about it.

Please sign in to comment.