Skip to content

Commit

Permalink
Change the way we do Int64 conversion to/from GAP
Browse files Browse the repository at this point in the history
Previously, any Int64 too big to fit into a GAP immediate integer was put into
a T_JULIA wrapper when passed to GAP, and unwrapped later. At the same time,
`julia_to_gap` relied completely (and incorrectly) on the C kernel functions
`julia_gap` and `gap_julia` to take care of *all* Int64 values.

The result was that passing such an integer as argument to a GAP function did
not work, not even if one explicitly used `julia_to_gap`.

To fix this we do two things:

1. We teach the C kernel function `gap_julia` to convert "large" Int64 values
   to GAP big integers; this ensures that the many adapter functions we
   install for Int64 arguments work.

2. We teach the Julia function `julia_to_gap` to perform a range check on
   Int64 values, and deal with those too big to fit into an immediate integer
   separately. As it turns out, Julia generates quite efficient machine code
   for the range check, so performance should be fine. We also apply the same
   improvement to a few other integer types.

The main drawback of that is that julia_to_gap(x::Int64) now is not type stable,
and thus has to return a boxed Int64 value. But that's not an in issue in so far
as that already now, whenever we call into GAP APIs, we have to box all Int64;
so we only change the point at which we box it.

Also, we violate the principle of roundtrip type fidelity (Int64 -> GAP large
int -> MPtr), but that is just a minor concern.

For really efficient conversions of e.g. Array{Int64,1} to GAP plists, we need
dedicated conversion functions which directly copy over bits. These can be
added transparently in the future.
  • Loading branch information
fingolfin committed Mar 5, 2020
1 parent 33f0587 commit c149b1b
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 16 deletions.
2 changes: 1 addition & 1 deletion pkg/GAPJulia/JuliaInterface/gap/convert.gd
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@
#! <List>
#! <Item>
#! <C>Int64</C> to immediate integer when it fits,
#! otherwise to &Julia; object wrapper,
#! otherwise to a &GAP; large integer,
#! </Item>
#! <Item>
#! <C>GapFFE</C> to immediate FFE,
Expand Down
2 changes: 1 addition & 1 deletion pkg/GAPJulia/JuliaInterface/src/convert.c
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ Obj gap_julia(jl_value_t * julia_obj)
if (INT_INTOBJ_MIN <= v && v <= INT_INTOBJ_MAX) {
return INTOBJ_INT(v);
}
return NewJuliaObj(julia_obj);
return ObjInt_Int8(v);
}
if (is_gapobj(julia_obj)) {
return (Obj)julia_obj;
Expand Down
39 changes: 25 additions & 14 deletions src/julia_to_gap.jl
Original file line number Diff line number Diff line change
Expand Up @@ -11,28 +11,39 @@ of input data, by converting egal data to identical objects in GAP.
"""


## Default
## Default for actual GAP objects is to do nothing
julia_to_gap(x::GapObj) = x
julia_to_gap(x::FFE) = x
julia_to_gap(x::Bool) = x

## Integers: general case first deal with things that fit into immediate
## integers, then falls back to converting to BigInt and calling into the GAP
## kernel API.
## TODO: we could provide more efficient conversion for UInt64, Int128, UInt128
## which avoids the conversion to BigInt, if we wanted to.
function julia_to_gap(x::Integer)
# if it fits into a GAP immediate integer, convert x to Int64
if x in -1<<60:(1<<60-1)
return Int64(x)
end
# for the general case, fall back to BigInt
return julia_to_gap(BigInt(x))
end

## Integers
julia_to_gap(x::Int128) = MakeObjInt(BigInt(x)) # FIXME: inefficient hack
julia_to_gap(x::Int64) = x
## Small integers types always fit into GAP immediate integers, and thus are
## represented by Int64 on the Julia side.
julia_to_gap(x::Int32) = Int64(x)
julia_to_gap(x::Int16) = Int64(x)
julia_to_gap(x::Int8) = Int64(x)

## Unsigned Integers
julia_to_gap(x::UInt128) = MakeObjInt(BigInt(x)) # FIXME: inefficient hack
julia_to_gap(x::UInt64) = MakeObjInt(BigInt(x)) # FIXME: inefficient hack
julia_to_gap(x::UInt32) = Int64(x)
julia_to_gap(x::UInt16) = Int64(x)
julia_to_gap(x::UInt8) = Int64(x)

## BigInts
julia_to_gap(x::BigInt) = MakeObjInt(x)
julia_to_gap(x::UInt32) = Int64(x)
julia_to_gap(x::UInt16) = Int64(x)
julia_to_gap(x::UInt8) = Int64(x)

## BigInts are converted via a ccall
function julia_to_gap(x::BigInt)
o = ccall(:MakeObjInt, Ptr{Cvoid}, (Ptr{UInt64},Cint), x.d, x.size)
return RAW_GAP_TO_JULIA(o)
end

## Rationals
function julia_to_gap(x::Rational{T}) where T <: Integer
Expand Down
15 changes: 15 additions & 0 deletions test/conversion.jl
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,21 @@ end
@test GAP.julia_to_gap(Int16(1)) == 1
@test GAP.julia_to_gap(Int8(1)) == 1

## Int64 corner cases
@test GAP.julia_to_gap(-2^60 ) === -2^60
@test GAP.julia_to_gap( 2^60-1) === 2^60 - 1

@test GAP.Globals.IsInt(GAP.julia_to_gap(-2^60-1))
@test GAP.Globals.IsInt(GAP.julia_to_gap( 2^60))

@test GAP.Globals.IsInt(GAP.julia_to_gap(-2^63-1))
@test GAP.Globals.IsInt(GAP.julia_to_gap(-2^63))
@test GAP.Globals.IsInt(GAP.julia_to_gap( 2^63-1))
@test GAP.Globals.IsInt(GAP.julia_to_gap( 2^63))

# see issue https://github.com/oscar-system/GAP.jl/issues/332
@test 2^60 * GAP.Globals.Factorial(20) == GAP.EvalString("2^60 * Factorial(20)")

## Unsigned integers
@test GAP.julia_to_gap(UInt128(1)) == 1
@test GAP.julia_to_gap(UInt64(1)) == 1
Expand Down

0 comments on commit c149b1b

Please sign in to comment.