itrunc -> trunc, etc, fix Int128 vs float comparisons

[simonbyrne]

This merges itrunc into trunc, iceil into ceil, ifloor into floor and iround into round, implementing most of #8728 (and fixes #3040). Some other changes were made along the way:

• trunc(Int,x) is bounds checked. The old "fast-but-unsafe" itrunc behaviour is now available via Base.unsafe_trunc (I'm open to better suggestions for names)
• Floating point <-> 128-bit integer conversions now use standard llvm intrinsics when possible. These seem to be faster and more accurate (there were some double-rounding issues in the old code). From what I can tell, these call out to libc functions such as __floattidf and __fixdfti. If these aren't available on all platforms, it should be fairly easy to implement them in Julia (e.g. here).
• Fixed floating point vs 128-bit integer comparisons. All float vs integer comparisons should now be correct (cf pi < pi gives error #8411).
• Fixed itrunc(::BigFloat), which was doing rounding, not truncation (e.g. itrunc(big(2.7)) = 3).
• round(Int,x) is implemented using the +prevfloat(0.5) trick mentioned in Add LLVM intrinsics for floor/ceil/trunc/abs. #8364 (comment). This does depend on rounding modes, but I could change it to using the +(x-trunc(x)) method once we have a fast trunc (Add LLVM intrinsics for floor/ceil/trunc/abs. #8364).

Sorry for the mammoth PR. I didn't intend to do all this at once, but it was difficult to split apart.

[tkelman]

Strange that this seems to work for me locally on linux64 and win64, but fails on Travis at the random tests - something to do with the ziggurat tables. And on linux32 and win32 it segfaults during bootstrap at float.jl:

float.jl

Please submit a bug report with steps to reproduce this fault, and any error messages that follow (in their entirety). Thanks.
Exception: EXCEPTION_ACCESS_VIOLATION at 0x0 -- unknown function (ip: 0)
unknown function (ip: 0)
convert at float.jl:10
unknown function (ip: 1644500959)
unknown function (ip: 1644785294)
unknown function (ip: 1644523418)
unknown function (ip: 1644530754)
unknown function (ip: 1644500959)
unknown function (ip: 1644785294)
unknown function (ip: 1644803759)
unknown function (ip: 1644806047)
unknown function (ip: 1644806713)
unknown function (ip: 1644500854)
unknown function (ip: 1644732978)
unknown function (ip: 1644730602)
unknown function (ip: 1644803567)
unknown function (ip: 1644804991)
unknown function (ip: 1644804328)
unknown function (ip: 1644806047)
unknown function (ip: 1644806547)
unknown function (ip: 4200027)
unknown function (ip: 4200480)
unknown function (ip: 1644756729)
unknown function (ip: 4202922)
unknown function (ip: 4199408)
unknown function (ip: 1971008394)
unknown function (ip: 2010292082)
unknown function (ip: 2010292037)
/bin/sh: line 1:  9516 Segmentation fault      /home/Tony/julia32/usr/bin/julia.exe --build `cygpath -w /home/Tony/julia32/usr/lib/julia/sys0` sysimg.jl
Makefile:142: recipe for target '/home/Tony/julia32/usr/lib/julia/sys0.o' failed
make[1]: *** [/home/Tony/julia32/usr/lib/julia/sys0.o] Error 139

[simonbyrne]

I'm looking into the random tests now: it seems to be a recent change, as I didn't get the errors until I rebased.

I suspect the 32-bit problems might have something to do with using the built-in intrinsics for conversions. Unfortunately I don't have access to a 32-bit test machine. Is there a way I can force a 32-bit build on a 64-bit machine?

[tkelman]

On Linux, kinda-sorta-mostly. Put the following in Make.user and ignore any failures in the libgit2 tests (I haven't gotten 64-to-32-bit compilation working with cmake yet).

override ARCH = i686
override MARCH = pentium4

Best to do this in a fresh clone since it'll need to rebuild 32-bit versions of all the deps. override OPENBLAS_DYNAMIC_ARCH = 0 will save a little bit of build time there.

[tkelman]

I'm looking into the random tests now: it seems to be a recent change, as I didn't get the errors until I rebased.

Oh and that explains the difference between running it locally vs on travis, since travis runs on a test merge commit of a PR into master. cc @rfourquet who has made a bunch of recent changes related to the RNG and might be able to help here.

[simonbyrne]

I think I've figured out the problem with the random tests: it's calling itrunc on BigFloats, so you're hitting the fix to the bug I mentioned above. I'm not sure what the tests are actually doing, but it does mean that whatever they're checking is actually incorrect. Should I file a separate bug?

[simonbyrne]

@andreasnoack is this a problem with the Ziggurat tables?

[andreasnoack]

The test checks that the generated tables are similar identical to the hardcoded tables. Maybe your changes affect some of the values in the generated tables. Probably, you can just update the hardcodes tables with this pull request. I can run BigCrush on the pull request afterwards to check that the new tables are fine.

[simonbyrne]

@andreasnoack Done.

[StefanKarpinski]

This is great – I've been wanting to do this for quite a while. I'm still surprised by how much nicer it feels.

[StefanKarpinski]

The fact that the ziggurat tables were wrong is really unsettling though.

[nalimilan]

Maybe the failure of random tests on 32-bit is #9107?

[tkelman]

@nalimilan it's not failing tests on 32 bit, it's segfaulting during system image bootstrap.

Fixed itrunc(::BigFloat), which was doing rounding, not truncation (e.g. itrunc(big(2.7)) = 3).

This is the bug you meant right? That is worrying. It would have maybe made sense to split the bugfix apart from the API change here, but too late now I suppose.

[simonbyrne]

@tkelman that's probably not a bad idea, because I guess this would be desirable for backporting. I'll see what I can do.

[simonbyrne]

I'm not having any luck getting the 32-bit build to work. I'll try rewriting the offending functions and see how I go.

[tkelman]

On Linux or OSX? If you're on Linux it should mostly work and I can have a go at fixing whatever's wrong, no idea about OSX though.

[simonbyrne]

@tkelman I keep getting openlibm errors: lots of lines like

/usr/bin/ld: i386:x86-64 architecture of input file `bsdsrc/b_exp.c.o' is incompatible with i386 output

[tkelman]

Was that in an existing build? You'll have to make -C deps distclean-openlibm and probably the same for all the other deps, and at that point it's better to make a totally separate clone for 32 bit.

[simonbyrne]

I tried that as well, then LLVM fails:

llvm[4]: Compiling APFloat.cpp for Release build
In file included from /home/simon/src/julia33/deps/llvm-3.3/include/llvm/Support
/SwapByteOrder.h:20:0,
                 from /home/simon/src/julia33/deps/llvm-3.3/include/llvm/Support
/MathExtras.h:17,
                 from /home/simon/src/julia33/deps/llvm-3.3/include/llvm/ADT/Sma
llVector.h:19,   
                 from /home/simon/src/julia33/deps/llvm-3.3/include/llvm/ADT/ArrayRef.h:14,
                 from /home/simon/src/julia33/deps/llvm-3.3/include/llvm/ADT/APInt.h:18,
                 from /home/simon/src/julia33/deps/llvm-3.3/include/llvm/ADT/APFloat.h:104,
                 from /home/simon/src/julia33/deps/llvm-3.3/lib/Support/APFloat.cpp:15:
/usr/include/c++/4.8/limits:1405:35: error: template argument 1 is invalid
     struct numeric_limits<__int128>
                                   ^
/usr/include/c++/4.8/limits:1479:44: error: template argument 1 is invalid
     struct numeric_limits<unsigned __int128>
                                            ^

[andreasnoack]

@StefanKarpinski The tables were not wrong. The variates passed BigCrush which is the only thing we can really use to verify them. There is room enough among the double precision numbers to move the table borders a single float here and there.

@simonbyrne Just ran BigCrush on PR and the tests passed.

[tkelman]

@andreasnoack could you try it on release-0.3 with the patch from #9138 applied? git checkout release-0.3 && curl https://github.com/JuliaLang/julia/pull/9138.patch | sed 's/UInt/Uint/g' | git apply --whitespace=fix

[andreasnoack]

@tkelman The tests just finished and they all passed.

[tkelman]

Wonderful. Were these the same ones that were failing before (#6464)? Or is uniform still messed up and these are just errors in the last place that subtly distorted the distribution?

[andreasnoack]

The tricky thing is that rand fails consistently, but transforming the variates back and forth to gaussians makes the tests pass consistently. It has been like that all the time and caused me some headache until I realized it.

[tkelman]

Okay, well sounds like we should merge #9138 anyway.

@simonbyrne I've never seen that one before but it looks like you might need to install gcc-multilib? Package naming depends on your distribution. https://gcc.gnu.org/ml/gcc-help/2014-01/msg00002.html

[simonbyrne]

From what I can tell, gcc-multilib is installed.

[simonbyrne]

@tkelman I've now implemented all the 128-bit conversions in native Julia, so would you be able to try the 32-bit machines again?

[tkelman]

sure. on 32 bit linux I get float64(int128(-1)) returning -0.0.

edit: same on win32. if you can't get building with -m32 (via ARCH=i686) to work, maybe a 32 bit VM is the best option - try https://vagrantcloud.com/ubuntu/boxes/trusty32. I had a thought that docker might be faster but apparently docker dgaf right now about 32 bit arches. I'll try again to get Travis to do a 32 bit build.

[tkelman]

I've found vagrant makes VM's super easy to use, FWIW.

Your last commit here 2322a7c passes tests for me on linux32 and win64, and although it brings #8895 back on win32 I suspect that's a sign of some deeper underlying problem with sys.dll rather than the fault of the changes here. With sys.dll deleted it also passes on win32.

I'll leave it to others to make the call on this, but looks okay to me right now.

[JeffBezanson]

Should this be trunc(Int,pr)? isodd is used on the result below. (Or trunc(Integer,pr) I guess.)

[simonbyrne]

No this was intentional. ip is only used in a comparison with pr, so it doesn't really make sense to convert to an integer, and then back again.

On second thoughts, this should really just be an isinteger...

[JeffBezanson]

That sounds good. Then we just need to take care of the isodd, which isn't defined for floating point and should probably stay that way.

[simonbyrne]

Ahh, missed that one. Should be fixed now.

[JeffBezanson]

💯 This is an awesome change.

[JeffBezanson]

Ok, I think this is ready to go. Maybe also a NEWS item?

[simonbyrne]

done.

[timholy]

I don't know whether this is fair to ask, but certainly I wouldn't mind if this type of change went along with an enhancement to Compat.jl. In this case, I did it myself, see JuliaLang/Compat.jl#20.

[StefanKarpinski]

I do think we need to start pairing breaking changes and deprecations with Compat support for same.

[timholy]

Does anyone besides me find the recommendation to use trunc(Integer, x) rather than trunc(Int, x) ever-so-slightly misguided? I worry that users might start declaring Array(Integer, 5) instead of Array(Int, 5).

[StefanKarpinski]

It's necessary for correctness of the deprecation definition, e.g. to replace itrunc(::BigFloat).

[timholy]

Thanks for explaining.

[ivarne]

As far as I can tell, the only limitation is the flexibility of the current @deprecate macro. We might improve it or deprecate manually, if we think the warning message should be better.