Skip to content

Commit

Permalink
use the right type (Limb) instead of Culong for limbs
Browse files Browse the repository at this point in the history
  • Loading branch information
@rfourquet
rfourquet committed Dec 2, 2014
1 parent e209872 commit 8b0921c
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 10 deletions.
12 changes: 11 additions & 1 deletion base/gmp.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,21 @@ end
typealias CdoubleMax Union(Float16, Float32, Float64)

const GMP_VERSION = VersionNumber(bytestring(unsafe_load(cglobal((:__gmp_version, :libgmp), Ptr{Cchar}))))
const GMP_BITS_PER_LIMB = Int(unsafe_load(cglobal((:__gmp_bits_per_limb, :libgmp), Cint)))

if GMP_BITS_PER_LIMB == 32
typealias Limb UInt32
elseif GMP_BITS_PER_LIMB == 64
typealias Limb UInt64
else
error("GMP: cannot determine the type mp_limb_t")
end

This comment has been minimized.

Sign in to view

Copy link
@ivarne

ivarne Dec 2, 2014

Member

This code will be executed at system image build time, not at runtime. I've seen discussions about distributing sys.{dll,so,dylib}. Should we just hope they distribute libgmp (or do something make that required somehow), so that it will be compiled with the same options on the target system? If we can't assume that, we'll need to put these checks in a __init__ function, and somehow figure out to write the code without compile time knowledge of the length of Limb

I don't expect @rfourquet to answer, but maybe others have some insight.

This comment has been minimized.

Sign in to view

Copy link
@ViralBShah

ViralBShah Dec 2, 2014

Member

Yes, that will not be easy. The original goal was to just support distros with both, GMP5 and GMP6, since people will have old distros that may not have GMP6.

This comment has been minimized.

Sign in to view

Copy link
@nalimilan

nalimilan Dec 2, 2014

Member

@ivarne Who is "they"? As regards Windows, I guess it's OK since the official build contains libgmp, right? For Linux, distributors take care of keeping packages in sync, so that's OK too. I suspect this is the same for OS X since the Homebrew packages are handled by @staticfloat. And isn't this a more general issue, for example as regards the length of BLAS integers too?

If supporting GMP5 is a problem here, it's fine with me to require version 6. But I don't see how it's related.

This comment has been minimized.

Sign in to view

Copy link
@ivarne

ivarne Dec 2, 2014

Member

It's probably a more general issue, that everybody is used to solving anyway. This just looked like code that maybe should be in __init__ instead of in global scope. I'd love to be wrong, because I can't see a pretty way of doing this check inside a function.

This comment has been minimized.

Sign in to view

Copy link
@rfourquet

rfourquet Dec 2, 2014

Author Member

There are two global constant now. It would probably be easier to put the BITS_PER_LIMB in __init__ than GMP_VERSION (it would be possible, but ugly and maybe with an overhead). But can this wait till a sys.* is distributed?

This comment has been minimized.

Sign in to view

Copy link
@JeffBezanson

JeffBezanson Dec 2, 2014

Member

Putting this in __init__ seems to entirely prevent precompiling code that uses GMP. What we could do is check in __init__ that the gmp in use matches what we built with.

This comment has been minimized.

Sign in to view

Copy link
@ivarne

ivarne Dec 2, 2014

Member

Thanks! It's so obvious in retrospect.

type BigInt <: Integer
alloc::Cint
size::Cint
d::Ptr{Culong}
d::Ptr{Limb}
function BigInt()
b = new(zero(Cint), zero(Cint), C_NULL)
ccall((:__gmpz_init,:libgmp), Void, (Ptr{BigInt},), &b)
Expand Down
19 changes: 10 additions & 9 deletions base/random.jl
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
module Random

using Base: dSFMT, Base.GMP.GMP_VERSION
using Base.dSFMT
using Base.GMP: GMP_VERSION, Limb

export srand,
rand, rand!,
Expand Down Expand Up @@ -391,17 +392,17 @@ if GMP_VERSION.major >= 6
a::BigInt # first
m::BigInt # range length - 1
nlimbs::Int # number of limbs in generated BigInt's
mask::Culong # applied to the highest limb
mask::Limb # applied to the highest limb
end

else
immutable RangeGeneratorBigInt <: RangeGenerator
a::BigInt # first
m::BigInt # range length - 1
limbs::Array{Culong} # buffer to be copied into generated BigInt's
mask::Culong # applied to the highest limb
limbs::Array{Limb} # buffer to be copied into generated BigInt's
mask::Limb # applied to the highest limb

RangeGeneratorBigInt(a, m, nlimbs, mask) = new(a, m, Array(Culong, nlimbs), mask)
RangeGeneratorBigInt(a, m, nlimbs, mask) = new(a, m, Array(Limb, nlimbs), mask)
end
end

Expand All @@ -411,9 +412,9 @@ function RangeGenerator(r::UnitRange{BigInt})
m = last(r) - first(r)
m < 0 && error("range must be non-empty")
nd = ndigits(m, 2)
nlimbs, highbits = divrem(nd, 8*sizeof(Culong))
nlimbs, highbits = divrem(nd, 8*sizeof(Limb))
highbits > 0 && (nlimbs += 1)
mask = highbits == 0 ? ~zero(Culong) : one(Culong)<<highbits - one(Culong)
mask = highbits == 0 ? ~zero(Limb) : one(Limb)<<highbits - one(Limb)
return RangeGeneratorBigInt(first(r), m, nlimbs, mask)
end

Expand Down Expand Up @@ -450,7 +451,7 @@ if GMP_VERSION.major >= 6
x = BigInt()
while true
# note: on CRAY computers, the second argument may be of type Cint (48 bits) and not Clong
xd = ccall((:__gmpz_limbs_write, :libgmp), Ptr{Culong}, (Ptr{BigInt}, Clong), &x, g.nlimbs)
xd = ccall((:__gmpz_limbs_write, :libgmp), Ptr{Limb}, (Ptr{BigInt}, Clong), &x, g.nlimbs)
limbs = pointer_to_array(xd, g.nlimbs)
rand!(rng, limbs)
limbs[end] &= g.mask
Expand All @@ -468,7 +469,7 @@ else
g.limbs[end] &= g.mask
ccall((:__gmpz_import, :libgmp), Void,
(Ptr{BigInt}, Csize_t, Cint, Csize_t, Cint, Csize_t, Ptr{Void}),
&x, length(g.limbs), -1, sizeof(Culong), 0, 0, &g.limbs)
&x, length(g.limbs), -1, sizeof(Limb), 0, 0, &g.limbs)
x <= g.m && break
end
ccall((:__gmpz_add, :libgmp), Void, (Ptr{BigInt}, Ptr{BigInt}, Ptr{BigInt}), &x, &x, &g.a)
Expand Down

5 comments on commit 8b0921c

@ViralBShah
Copy link
Member

Choose a reason for hiding this comment

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

Should Limb types correspond to C types, instead of Julia types?

@ivarne
Copy link
Member

@ivarne ivarne commented on 8b0921c Dec 2, 2014

Choose a reason for hiding this comment

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

I don't think so. The C types are just typealiases for Julia types to make it more convenient to write C interfaces without conditionals all over the code to have correct integer sizes on 32 and 64 bit systems. Here we know the number of bits in the type, and we can directly use Julia types with the correct number of bits.

@rfourquet
Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with what @ivarne said. According to GMP sources, the Limb could be unsigned, unsigned long or unsigned long long. So I assumed here for simplicity that sizeof(unsigned) >= 32 and sizeof(unsigned long long) <= 64.

@ivarne
Copy link
Member

@ivarne ivarne commented on 8b0921c Dec 2, 2014

Choose a reason for hiding this comment

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

You have not just assumed, you have asserted and will crash the sysimg build with a (useful?) error if it is not true. (which is the right way to do it.)

I'm generally in favor of including the invalid value in errors like that. How about

error("GMP: cannot determine the type mp_limb_t (__gmp_bits_per_limb == $GMP_BITS_PER_LIMB)")

That typically saves us one roundtrip of questions when someone asks for help debugging when the error is triggered.

@rfourquet
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes you are right, I will change this message when squashing.

Please sign in to comment.