Skip to content
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
Merged

WIP: checked integer conversions #8420

merged 17 commits into from
Sep 29, 2014

Conversation

JeffBezanson
Copy link
Member

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

  1. fixes Type changes in scalar computation #3759, making arithmetic type-preserving
  2. fixes convert truncates integers #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.

…> T (#3759)

the sysimg builds, but still need to:
  - check same-size signed<->unsigned conversion
  - make tests pass
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)))
Copy link
Member

Choose a reason for hiding this comment

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

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

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 but I believe it is needed to be able to execute the for loop.

@timholy
Copy link
Member

timholy commented Sep 20, 2014

Seems to include #8028.

@timholy
Copy link
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
Copy link
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
Copy link
Member

IainNZ commented Sep 20, 2014

I can certainly do that if it'd be helpful

@vtjnash vtjnash mentioned this pull request Sep 22, 2014
a few fixes in base for the convert and arithmetic changes

get all tests passing
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
Copy link
Member Author

I believe this is done now.

@timholy
Copy link
Member

timholy commented Sep 25, 2014

I'm still a little worried about reduce.jl. I haven't analyzed as carefully as you probably have by now, so I could be wrong here, but points of concern for me are here, here, here, here.

Of course, some of those optimizations might be rolled back anyway, in light of the recent SIMD advances. It would be nice to have it be simpler...

@StefanKarpinski
Copy link
Member

I'm also concerned about reductions producing the same type. I feel like reductions over small types should produce Int/Uint.

@IainNZ
Copy link
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
Copy link
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.

@eschnett
Copy link
Contributor

See #8524

@rickhg12hs
Copy link
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)

@armgong
Copy link
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
Copy link
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

@timholy
Copy link
Member

timholy commented Sep 30, 2014

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

@simonster simonster mentioned this pull request Oct 1, 2014
simonster added a commit to JuliaCollections/SortingAlgorithms.jl that referenced this pull request Oct 1, 2014
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 October 10, 2014 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

convert truncates integers Type changes in scalar computation