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.

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

WIP: checked integer conversions #8420

Merged
merged 17 commits into from Sep 29, 2014

Conversation

Projects
None yet
@JeffBezanson
Member

JeffBezanson commented Sep 19, 2014

This is not done yet but ready for discussion at least.

  1. fixes #3759, making arithmetic type-preserving
  2. fixes #5413, checking integer truncations
  3. also checks signed<->unsigned conversions

(1) was simple and did not really break much.
(2) was a bit more breaking, but not too bad and leads to clearer code in most cases.
(3) was kind of a disaster and seems to be very annoying.

The problem with (3) is that interpreting a 2's complement integer as unsigned is mathematically useful. It also comes up a lot in hashing, random number generation, and when you just want to write a byte to an IO stream.

I'm not sure yet how to rewrite

reinterpret(T, Base.asunsigned(g.a) + rem_knuth(x, g.k))
with checked integer conversions. We need to decide how to handle cases like this.

I think adding these checks is clearly the right thing, but I was surprised at how many legitimate uses of "unsafe" integer conversion there were. I added methods to itrunc for unsafely truncating integers, but this only handles converting from larger to smaller. We need to figure out what other functions might be needed.

JeffBezanson added some commits Sep 17, 2014

check integer truncation (#5413) and make more operators follow T+T =…
…> T (#3759)

the sysimg builds, but still need to:
  - check same-size signed<->unsigned conversion
  - make tests pass
@StefanKarpinski

This comment has been minimized.

Show comment
Hide comment
@StefanKarpinski

StefanKarpinski Sep 17, 2014

Member

Is the idea to simultaneously making IntN + IntN => IntN and making convert(IntN, x::IntM) check for overflow when N < M?

Member

StefanKarpinski commented on 1d5050b Sep 17, 2014

Is the idea to simultaneously making IntN + IntN => IntN and making convert(IntN, x::IntM) check for overflow when N < M?

This comment has been minimized.

Show comment
Hide comment
@JeffBezanson

JeffBezanson Sep 17, 2014

Member

Yes. Though I suppose the first of those changes is much simpler and could be done separately.

Member

JeffBezanson replied Sep 17, 2014

Yes. Though I suppose the first of those changes is much simpler and could be done separately.

JeffBezanson added some commits Sep 17, 2014

check signed<->unsigned conversions
this would be a bit cleaner without the code in intfuncs that dispatches
on Unsigned.

signed() and unsigned() are convenient for testing bit patterns in the
test suite. for now replace with assigned() and asunsigned().

uint8(x) is convenient for moving bytes around, so it remains unchecked.
we should perhaps rename this to byte(), and either eliminate uint8()
entirely, or make it checked.
## integer arithmetic ##
+(x::Int, y::Int) = box(Int,add_int(unbox(Int,x),unbox(Int,y)))

This comment has been minimized.

@vtjnash

vtjnash Sep 19, 2014

Member

isn't this a duplicate of one of the definitions in the for eval loop below?

@vtjnash

vtjnash Sep 19, 2014

Member

isn't this a duplicate of one of the definitions in the for eval loop below?

This comment has been minimized.

@JeffBezanson

JeffBezanson Sep 19, 2014

Member

Yes but I believe it is needed to be able to execute the for loop.

@JeffBezanson

JeffBezanson Sep 19, 2014

Member

Yes but I believe it is needed to be able to execute the for loop.

Show outdated Hide outdated base/int.jl
Show outdated Hide outdated base/int.jl
uint8(x) = convert(Uint8,x)
uint8(x) = convert(Uint8, x)
uint8(x::Integer) = itrunc(Uint8,x)
uint8(x::Int8) = box(Uint8,unbox(Int8,x))

This comment has been minimized.

@vtjnash

vtjnash Sep 19, 2014

Member

why is this a special case (no overflow error)?

@vtjnash

vtjnash Sep 19, 2014

Member

why is this a special case (no overflow error)?

This comment has been minimized.

@JeffBezanson

JeffBezanson Sep 19, 2014

Member

Because of the way I/O uses this (see my comments). Everybody wants to get rid of the "lowercase conversion" functions anyway, so we need a better solution for unsafe conversion to uint8.

@JeffBezanson

JeffBezanson Sep 19, 2014

Member

Because of the way I/O uses this (see my comments). Everybody wants to get rid of the "lowercase conversion" functions anyway, so we need a better solution for unsafe conversion to uint8.

This comment has been minimized.

@vtjnash

vtjnash Sep 19, 2014

Member

coerce? either as written below, or adding an intrinsic to do the size test

@vtjnash

vtjnash Sep 19, 2014

Member

coerce? either as written below, or adding an intrinsic to do the size test

@eval typemin(::Type{Uint128}) = $(uint128(0))
@eval typemax(::Type{Uint128}) = $(box(Uint128,unbox(Int128,convert(Int128,-1))))
@eval typemin(::Type{Int128} ) = $(convert(Int128,1)<<int32(127))
@eval typemax(::Type{Int128} ) = $(box(Int128,unbox(Uint128,typemax(Uint128)>>int32(1))))

This comment has been minimized.

@vtjnash

vtjnash Sep 19, 2014

Member

why are these @eval? are you just concerned this creates extra work for the inliner / llvm? it seems the resulting code should be unconditionally the same, after llvm's constant propagation pass

@vtjnash

vtjnash Sep 19, 2014

Member

why are these @eval? are you just concerned this creates extra work for the inliner / llvm? it seems the resulting code should be unconditionally the same, after llvm's constant propagation pass

This comment has been minimized.

@JeffBezanson

JeffBezanson Sep 19, 2014

Member

You're probably right but yes, I don't want to tax our inlining heuristics with this.

@JeffBezanson

JeffBezanson Sep 19, 2014

Member

You're probably right but yes, I don't want to tax our inlining heuristics with this.

xy == xy8 || throw(OverflowError())
return xy8
xy = widemul(x,y)
(typemin($T) <= xy <= typemax($T)) || throw(OverflowError())

This comment has been minimized.

@vtjnash

vtjnash Sep 19, 2014

Member

it seems like this would be much faster if it utilized the llvm checked-multiply intrinsic, no?
llvm.smul.with.overflow

@vtjnash

vtjnash Sep 19, 2014

Member

it seems like this would be much faster if it utilized the llvm checked-multiply intrinsic, no?
llvm.smul.with.overflow

This comment has been minimized.

@JeffBezanson

JeffBezanson Sep 19, 2014

Member

See the comment right above. LLVM's checked multiply is broken for 8-bit.

@JeffBezanson

JeffBezanson Sep 19, 2014

Member

See the comment right above. LLVM's checked multiply is broken for 8-bit.

This comment has been minimized.

@vtjnash

vtjnash Sep 19, 2014

Member

ah, now i see it – it wasn't visible in my diff (perhaps because of the intervening blank line?)

for int8, i guess this isn't really that bad

it is broken for i64 on x86 too (the below case)?

@vtjnash

vtjnash Sep 19, 2014

Member

ah, now i see it – it wasn't visible in my diff (perhaps because of the intervening blank line?)

for int8, i guess this isn't really that bad

it is broken for i64 on x86 too (the below case)?

This comment has been minimized.

@Keno

Keno Sep 20, 2014

Member

No, just i8 and i>=128: llvm-mirror/llvm@0bedfa4

@Keno

Keno Sep 20, 2014

Member

No, just i8 and i>=128: llvm-mirror/llvm@0bedfa4

This comment has been minimized.

@vtjnash

vtjnash Sep 20, 2014

Member

so this is actually conditional on upgrading the version of julia?

@vtjnash

vtjnash Sep 20, 2014

Member

so this is actually conditional on upgrading the version of julia?

This comment has been minimized.

@Keno

Keno Sep 20, 2014

Member

using the intrinsic is, yes

@Keno

Keno Sep 20, 2014

Member

using the intrinsic is, yes

@@ -243,31 +243,31 @@ end
# conversions
function fooo()
local x::Int8
x = 1000
x = 100

This comment has been minimized.

@vtjnash

vtjnash Sep 19, 2014

Member

i think this test used to be checking for the llvm/clang behavior with respect to generating the necessary truncate instructions at the ccall boundary (call <struct jl_value_t*> @jl_box_int8(int8 100)). but perhaps that test isn't really needed after this change anyways?

@vtjnash

vtjnash Sep 19, 2014

Member

i think this test used to be checking for the llvm/clang behavior with respect to generating the necessary truncate instructions at the ccall boundary (call <struct jl_value_t*> @jl_box_int8(int8 100)). but perhaps that test isn't really needed after this change anyways?

@@ -16,9 +16,24 @@ vals = [
typemax(Int64),
]
function coerce(T::Type, x)
if T<:Rational

This comment has been minimized.

@vtjnash

vtjnash Sep 19, 2014

Member

coerce{T<:Rational}(::Type{T}, x) = convert(T, coerce(typeof(num(zero(T))), x)) ?

@vtjnash

vtjnash Sep 19, 2014

Member

coerce{T<:Rational}(::Type{T}, x) = convert(T, coerce(typeof(num(zero(T))), x)) ?

else
convert(T, x)
end
end

This comment has been minimized.

@vtjnash

vtjnash Sep 19, 2014

Member

for completeness:

coerce{T<:Rational}(::Type{T}, x) = convert(T, coerce(typeof(num(zero(T))), x))
function coerce{T<:Integer}(::Type{T}, x)
    if sizeof(T) < sizeof(x)
        itrunc(T, x)
    else if sizeof(T) == sizeof(x)
        reinterpret(T, x)
    else
        convert(T, x)
    end
end
coerce(::Type{Bool}, x) = convert(Bool, x)
coerce{T}(::Type{T}, x) = convert(T, x)

i think the above would generate better code, since it lets the compiler eliminate the tests and inline much better

@vtjnash

vtjnash Sep 19, 2014

Member

for completeness:

coerce{T<:Rational}(::Type{T}, x) = convert(T, coerce(typeof(num(zero(T))), x))
function coerce{T<:Integer}(::Type{T}, x)
    if sizeof(T) < sizeof(x)
        itrunc(T, x)
    else if sizeof(T) == sizeof(x)
        reinterpret(T, x)
    else
        convert(T, x)
    end
end
coerce(::Type{Bool}, x) = convert(Bool, x)
coerce{T}(::Type{T}, x) = convert(T, x)

i think the above would generate better code, since it lets the compiler eliminate the tests and inline much better

This comment has been minimized.

@vtjnash

vtjnash Sep 19, 2014

Member

oh, i see that this isn't in base, so it isn't as important. it seems like a pretty good name for assigned/asunsigned (e.g. coerce(Signed, x))

it's missing a test for isleaftype(T) before sizeof(T) which is needed if this does end up in base, and a few other possible definitions:

coerce(::Type{Unsigned}, x) = coerce(typeof(unsigned(zero(x))), x)
coerce(::Type{Signed}, x) = coerce(typeof(signed(zero(x))), x)
coerce{T}(::Type{T}, x::T) = x
@vtjnash

vtjnash Sep 19, 2014

Member

oh, i see that this isn't in base, so it isn't as important. it seems like a pretty good name for assigned/asunsigned (e.g. coerce(Signed, x))

it's missing a test for isleaftype(T) before sizeof(T) which is needed if this does end up in base, and a few other possible definitions:

coerce(::Type{Unsigned}, x) = coerce(typeof(unsigned(zero(x))), x)
coerce(::Type{Signed}, x) = coerce(typeof(signed(zero(x))), x)
coerce{T}(::Type{T}, x::T) = x
xy == xy64 || throw(OverflowError())
return xy64
(typemin($T) <= xy <= typemax($T)) || throw(OverflowError())
return itrunc($T,xy)

This comment has been minimized.

@vtjnash

vtjnash Sep 20, 2014

Member

so why was this here?

@vtjnash

vtjnash Sep 20, 2014

Member

so why was this here?

This comment has been minimized.

@Keno

Keno Sep 20, 2014

Member

Oh, sorry, it's everything > wordsize and this is on 32bit.

@Keno

Keno Sep 20, 2014

Member

Oh, sorry, it's everything > wordsize and this is on 32bit.

rand(::Type{Int32}) = int32(rand(Uint32))
rand(::Type{Int64}) = int64(rand(Uint64))
rand(::Type{Int128}) = int128(rand(Uint128))
rand(::Type{Int8}) = itrunc(Int8,rand(Uint32))

This comment has been minimized.

@timholy

timholy Sep 20, 2014

Member

I hadn't realized we were generating our low-bitcount random numbers this way. I presume someone knows that discarding is better than saving the other bytes somewhere for the next rand(Int8)?

@timholy

timholy Sep 20, 2014

Member

I hadn't realized we were generating our low-bitcount random numbers this way. I presume someone knows that discarding is better than saving the other bytes somewhere for the next rand(Int8)?

This comment has been minimized.

@vtjnash

vtjnash Sep 20, 2014

Member

i think that's where filling an array can be more efficient than asking for them one at a time

@vtjnash

vtjnash Sep 20, 2014

Member

i think that's where filling an array can be more efficient than asking for them one at a time

This comment has been minimized.

@JeffBezanson

JeffBezanson Sep 20, 2014

Member

Yes, for filling an array it could be faster. I tried saving the extra bits for individual values once, but couldn't make it fast enough to make up for the extra bookkeeping overhead.

@JeffBezanson

JeffBezanson Sep 20, 2014

Member

Yes, for filling an array it could be faster. I tried saving the extra bits for individual values once, but couldn't make it fast enough to make up for the extra bookkeeping overhead.

This comment has been minimized.

@timholy

timholy Sep 20, 2014

Member

I'm not surprised you tried. Makes sense then to keep it like this.

@timholy

timholy Sep 20, 2014

Member

I'm not surprised you tried. Makes sense then to keep it like this.

@timholy

This comment has been minimized.

Show comment
Hide comment
@timholy

timholy Sep 20, 2014

Member

Seems to include #8028.

Member

timholy commented Sep 20, 2014

Seems to include #8028.

@timholy

This comment has been minimized.

Show comment
Hide comment
@timholy

timholy Sep 20, 2014

Member

@JeffBezanson, you also need to fix these. EDIT: also here, and I think quite a few other places in reduce.jl.

As I pointed out #8028 (comment), this change in behavior may not be caught by many tests. So I suspect the fact that (1) "did not break much" may not tell the whole story. I for one am sure I have a lot of code where I add small integers together, and now probably need to throw in a few widens.

To be clear, I completely support & endorse this change, but realistically I think this will be one of the most painful transitions in 0.4.

Member

timholy commented Sep 20, 2014

@JeffBezanson, you also need to fix these. EDIT: also here, and I think quite a few other places in reduce.jl.

As I pointed out #8028 (comment), this change in behavior may not be caught by many tests. So I suspect the fact that (1) "did not break much" may not tell the whole story. I for one am sure I have a lot of code where I add small integers together, and now probably need to throw in a few widens.

To be clear, I completely support & endorse this change, but realistically I think this will be one of the most painful transitions in 0.4.

@timholy

This comment has been minimized.

Show comment
Hide comment
@timholy

timholy Sep 20, 2014

Member

@IainNZ, once merger seems imminent (and I think this is very close to ready) this would be a good PR to prospectively run through PackageEval.

Member

timholy commented Sep 20, 2014

@IainNZ, once merger seems imminent (and I think this is very close to ready) this would be a good PR to prospectively run through PackageEval.

@IainNZ

This comment has been minimized.

Show comment
Hide comment
@IainNZ

IainNZ Sep 20, 2014

Member

I can certainly do that if it'd be helpful

Member

IainNZ commented Sep 20, 2014

I can certainly do that if it'd be helpful

@vtjnash vtjnash referenced this pull request Sep 22, 2014

Closed

checked math by default #8114

JeffBezanson added some commits Sep 25, 2014

make signed() and unsigned() unchecked. check only in convert()
a few fixes in base for the convert and arithmetic changes

get all tests passing
add a bit of widening to reductions, to make up for the new
type-preserving arithmetic.

allow reducedim(+, A, 2), which previously gave
```
ERROR: `reducedim_init` has no method matching reducedim_init(::IdFun, ::Function, ::Array{Uint8,2}, ::Int64)
 in reducedim at reducedim.jl:217
```
@JeffBezanson

This comment has been minimized.

Show comment
Hide comment
@JeffBezanson

JeffBezanson Sep 25, 2014

Member

I believe this is done now.

Member

JeffBezanson commented Sep 25, 2014

I believe this is done now.

@timholy

This comment has been minimized.

Show comment
Hide comment
@timholy

timholy Sep 25, 2014

Member

Putting back widen(Int8)->Int does seem prudent.

Member

timholy commented on base/int.jl in 16c2330 Sep 25, 2014

Putting back widen(Int8)->Int does seem prudent.

@IainNZ

This comment has been minimized.

Show comment
Hide comment
@IainNZ

IainNZ Sep 29, 2014

Member

Many failures, but hopefully mostly from JSON.jl: http://pkg.julialang.org/pulse.html
I've only filed issues for packages that did NOT fail due to a dependency. If they are hard-fails they typically depend on JSON, if they are full_fail they are often their own problems. There may be some packages that will fail even after JSON is fixed, but that is hard to say at this point.

You can click on the test status of a package on that pulse page to take you to the entry for that package, where you can view the log.

Member

IainNZ commented Sep 29, 2014

Many failures, but hopefully mostly from JSON.jl: http://pkg.julialang.org/pulse.html
I've only filed issues for packages that did NOT fail due to a dependency. If they are hard-fails they typically depend on JSON, if they are full_fail they are often their own problems. There may be some packages that will fail even after JSON is fixed, but that is hard to say at this point.

You can click on the test status of a package on that pulse page to take you to the entry for that package, where you can view the log.

@simonster

This comment has been minimized.

Show comment
Hide comment
@simonster

simonster Sep 29, 2014

Member

The JSON issue appears to be happening because it uses a Char range that starts with \x00, which is broken because of the subtraction here. Also, it seems like that line may sometimes do some unnecessary boxing because it's missing an oftype.

Member

simonster commented Sep 29, 2014

The JSON issue appears to be happening because it uses a Char range that starts with \x00, which is broken because of the subtraction here. Also, it seems like that line may sometimes do some unnecessary boxing because it's missing an oftype.

@eschnett

This comment has been minimized.

Show comment
Hide comment
@eschnett

eschnett Sep 29, 2014

Contributor

See #8524

Contributor

eschnett commented Sep 29, 2014

See #8524

@rickhg12hs

This comment has been minimized.

Show comment
Hide comment
@rickhg12hs

rickhg12hs Sep 30, 2014

Contributor

32-bit Fedora 19 Linux build is broken.

hashing.jl
error during bootstrap: LoadError(at "sysimg.jl" line 58: LoadError(at
"hashing.jl" line 68: InexactError()))
make[1]: *** [/usr/local/src/julia/julia/usr/lib/juli
a/sys0.o] Error 1
make: *** [release] Error 2

On 9/29/14, Tony Kelman notifications@github.com wrote:

fixed those (thanks @vtjnash) test failures, surprisingly no other win64
regressions.

win32 won't bootstrap, and I suspect 32-bit Linux won't either, due to

julia/base/hashing.jl

Lines 56 to 57 in 95042f1

hx(a::Uint64, b::Float64, h::Uint) = hash_uint64((3a + reinterpret(Uint64,b)) - h)
const hx_NaN = hx(uint64(0), NaN, uint(0 ))

error during bootstrap: LoadError(at "sysimg.jl" line 58: LoadError(at
"hashing.jl" line 57: InexactError()))

Reply to this email directly or view it on GitHub:
#8420 (comment)

Contributor

rickhg12hs commented Sep 30, 2014

32-bit Fedora 19 Linux build is broken.

hashing.jl
error during bootstrap: LoadError(at "sysimg.jl" line 58: LoadError(at
"hashing.jl" line 68: InexactError()))
make[1]: *** [/usr/local/src/julia/julia/usr/lib/juli
a/sys0.o] Error 1
make: *** [release] Error 2

On 9/29/14, Tony Kelman notifications@github.com wrote:

fixed those (thanks @vtjnash) test failures, surprisingly no other win64
regressions.

win32 won't bootstrap, and I suspect 32-bit Linux won't either, due to

julia/base/hashing.jl

Lines 56 to 57 in 95042f1

hx(a::Uint64, b::Float64, h::Uint) = hash_uint64((3a + reinterpret(Uint64,b)) - h)
const hx_NaN = hx(uint64(0), NaN, uint(0 ))

error during bootstrap: LoadError(at "sysimg.jl" line 58: LoadError(at
"hashing.jl" line 57: InexactError()))

Reply to this email directly or view it on GitHub:
#8420 (comment)

@yomichi yomichi referenced this pull request Sep 30, 2014

Merged

fixed test_bcast.jl #19

@armgong

This comment has been minimized.

Show comment
Hide comment
@armgong

armgong Sep 30, 2014

Contributor

32-bit windows build also broken (LLVM 3.3) ,but 64-bit windows build ok
base64.jl
io.jl
iostream.jl
libc.jl
env.jl
error during bootstrap: Makefile:124: recipe for target '/d/codes/julia32/usr/lib/julia/sys0.o' failed
make[1]: *** [/d/codes/julia32/usr/lib/julia/sys0.o] Error 1
Makefile:37: recipe for target 'release' failed
make: *** [release] Error 2

Contributor

armgong commented Sep 30, 2014

32-bit windows build also broken (LLVM 3.3) ,but 64-bit windows build ok
base64.jl
io.jl
iostream.jl
libc.jl
env.jl
error during bootstrap: Makefile:124: recipe for target '/d/codes/julia32/usr/lib/julia/sys0.o' failed
make[1]: *** [/d/codes/julia32/usr/lib/julia/sys0.o] Error 1
Makefile:37: recipe for target 'release' failed
make: *** [release] Error 2

@rickhg12hs

This comment has been minimized.

Show comment
Hide comment
@rickhg12hs

rickhg12hs Sep 30, 2014

Contributor

32-bit Fedora 19 Linux build broken in bootstrap with env.jl

env.jl
error during bootstrap: LoadError(at "sysimg.jl" line 98: LoadError(at "env.jl" line 133: InexactError()))
make[1]: *** [/usr/local/src/julia/julia/usr/lib/julia/sys0.o] Error 1
make: *** [release] Error 2
Contributor

rickhg12hs commented Sep 30, 2014

32-bit Fedora 19 Linux build broken in bootstrap with env.jl

env.jl
error during bootstrap: LoadError(at "sysimg.jl" line 98: LoadError(at "env.jl" line 133: InexactError()))
make[1]: *** [/usr/local/src/julia/julia/usr/lib/julia/sys0.o] Error 1
make: *** [release] Error 2
@timholy

This comment has been minimized.

Show comment
Hide comment
@timholy

timholy Sep 30, 2014

Member

This is shaping up to be a real 0.4 change 😉.

Member

timholy commented Sep 30, 2014

This is shaping up to be a real 0.4 change 😉.

@IainNZ IainNZ referenced this pull request Sep 30, 2014

Merged

make [] give an Any array #8493

@simonster simonster referenced this pull request Oct 1, 2014

Closed

remove uint_mapping? #8539

simonster added a commit to JuliaCollections/SortingAlgorithms.jl that referenced this pull request Oct 1, 2014

Move uint_mapping here from Base and fix it for JuliaLang/julia#8420
I also got rid of the manual inlining since the compiler now appears to
inline everything on its own.

Fixes #6. Ref JuliaLang/julia#8539

@JeffBezanson JeffBezanson deleted the jb/checked_int_trunc branch Oct 10, 2014

@quinnj

This comment has been minimized.

Show comment
Hide comment
@quinnj

quinnj Oct 13, 2014

Member

This is due to convert(Int64,x) now throwing an InexactError if x > typemax(Int64); correct?

Member

quinnj commented on test/dates/types.jl in c67837c Oct 13, 2014

This is due to convert(Int64,x) now throwing an InexactError if x > typemax(Int64); correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment