check for typemin(T) in denominator of rational

[simeonschaub]

fix #32569. This is my first PR here, I hope, I didn't miss anything.

[KlausC]

I am not happy with your use of isbitstype(T); please replace with Base.hastypemax(T). That is also the tenor of @rfourquet's comment, I think.

The former makes use of the reflection interface and produces runtime code like:

julia> @code_llvm isbitstype(Int)
;  @ reflection.jl:452 within isbitstype
define i8 @julia_isbitstype_16522(%jl_value_t addrspace(10)*) {
top:
; ┌ @ Base.jl:15 within getproperty
   %1 = load i8, i8* inttoptr (i64 140103251893833 to i8*), align 1
   %2 = and i8 %1, 1
;
  ret i8 %2
}

while the proposed call evaluates essentially to a constant at run time:

julia> @code_llvm Base.hastypemax(Int)
;  @ intfuncs.jl:726 within `hastypemax'
define i8 @julia_hastypemax_16523(%jl_value_t addrspace(10)*) {
top:
  ret i8 1
}

[simeonschaub]

Are you suggesting that I implement a hastypemin function, analogous to Base.hastypemax, or should I just assume that every Integer that has typemax defined also has typemin defined and just use hastypemax here?

[rfourquet]

Are you suggesting that I implement a hastypemin function, analogous to Base.hastypemax

Yes

[KlausC]

The implementation: hastypemin(x::Type{<:Integer}) = hastypemax(x) would cover all subtypes of Integer defined in Base correctly and with the same performance advantage.

BTW: Base.hastypemax is not implemented optimally for floating point types.

[rfourquet]

The implementation: hastypemin(x::Type{<:Integer}) = hastypemax(x) would cover all subtypes of Integer defined in Base correctly

But it's not valid in general, the definitions are tiny, let's not try to save duplication here at the cost of an approximation

[simeonschaub]

Bump.

[KlausC]

bump!
The rationals label is missing.

[rfourquet]

I'm sorry, in my previous review I was just looking at the typemin stuff, but actually I think this error logic here is not appropriate in general. A type could have a typemin without the current definition being an error. The real problem occurs when typemin(T) == -typemin(T) (which is typically true for bit types which use 2-complement arithmetic). So I suggest instead moving the error checking in the below line, something like: T <: Signed && signbit(den) && signbit(-den) && throw(...).

[simeonschaub]

Yes, makes sense. So just delete all the hastypemin stuff?

[KlausC]

I don't think, this check is very good, because it requires construction of -den, which is expensive in the case of BigInt.

[rfourquet]

It can be constructed only when necessary.

[KlausC]

I think the previous commit was better, because it does not require to construct the negative of a BigInt in that case. Timing examples:

julia> @btime Base.hastypemax(BigInt)
  0.016 ns (0 allocations: 0 bytes)
false

julia> @btime -$(big(1));
  54.518 ns (2 allocations: 40 bytes)

julia> x = big(51)^1000;
julia> @btime -$x;
  119.650 ns (3 allocations: 744 bytes)

[rfourquet]

As @KlausC suggested, this constructs unnecessarily -den once more than necessary. You can rearrange the code to fix that, e.g.

num2, den2 = if signbit(den)
    den = -den
    T <: Signed && signbit(den) && __throw_rational_argerror_typemin(T)
    divgcd(-num, den)
else
    divgcd(num, den)
end

[KlausC]

a little polished:

if T<:Signed && signbit(den)
    den = -den
    signbit(den) && __throw_rational_argerror_typemin(T)
    num = -num
end
num2, den2 = divgcd(num, den)

[KlausC]

@StefanKarpinski , what do you think?

[rfourquet]

Usually we don't insert references to issues in code (as opposed to tests), and here I think we can understand the reason of this test by following the thrown error, faster than by looking up github. No really a problem though, and probably not necessary to update your PR.

[simeonschaub]

That sounds reasonable. Went ahead and deleted it now.

[KlausC]

Additional issue? Are these test cases ok now?

@test -2 // typemin(Int) == 1//4611686018427387904
@test 2 // typemin(Int) == -1//4611686018427387904

maybe the divgcd has to be done before the checks.

[KlausC]

@simeonschaub, would you mind to insert the changes to fix - last comment?

[KlausC]

I am not sure, if the @show .. should be there.

The test cases are only valid if Int == Int64; should be formulated generically.

Do you run make test-rational locally?

[simeonschaub]

Oh, sorry. My bad! I pushed a bit too prematurely.

[rfourquet]

Looks like good to go, I will merge in one day or two if no other review comments (does it look good for you now @KlausC ?)

[rfourquet]

Thanks @simeonschaub for the fix and @KlausC for the review!