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

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

Merged
merged 5 commits into from
Nov 25, 2014

Conversation

simonbyrne
Copy link
Contributor

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
Copy link
Contributor

tkelman commented Nov 24, 2014

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
Copy link
Contributor Author

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
Copy link
Contributor

tkelman commented Nov 24, 2014

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
Copy link
Contributor

tkelman commented Nov 24, 2014

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@andreasnoack is this a problem with the Ziggurat tables?

@andreasnoack
Copy link
Member

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
Copy link
Contributor Author

@andreasnoack Done.

@StefanKarpinski
Copy link
Member

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

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

@nalimilan
Copy link
Member

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

@tkelman
Copy link
Contributor

tkelman commented Nov 24, 2014

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor

tkelman commented Nov 24, 2014

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
Copy link
Contributor Author

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

tkelman commented Nov 24, 2014

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
Copy link
Contributor Author

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

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

tkelman commented Nov 24, 2014

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

@tkelman The tests just finished and they all passed.

@tkelman
Copy link
Contributor

tkelman commented Nov 24, 2014

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

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
Copy link
Contributor

tkelman commented Nov 24, 2014

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
Copy link
Contributor Author

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

@simonbyrne
Copy link
Contributor Author

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

tkelman commented Nov 25, 2014

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
Copy link
Contributor

tkelman commented Nov 25, 2014

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.

@@ -477,7 +477,7 @@ function ^{T<:Complex}(z::T, p::T)

# apply some corrections to force known zeros
if pim == 0
ip = itrunc(pr)
ip = trunc(pr)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, missed that one. Should be fixed now.

@JeffBezanson
Copy link
Member

💯 This is an awesome change.

@JeffBezanson
Copy link
Member

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

@simonbyrne
Copy link
Contributor Author

done.

JeffBezanson added a commit that referenced this pull request Nov 25, 2014
itrunc -> trunc, etc, fix Int128 vs float comparisons
@JeffBezanson JeffBezanson merged commit 12ce837 into JuliaLang:master Nov 25, 2014
@timholy
Copy link
Member

timholy commented Nov 27, 2014

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

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

timholy added a commit to JuliaAttic/Color.jl that referenced this pull request Nov 30, 2014
See JuliaLang/julia#9133
This uses Compat to ensure backwards-compatibility
@timholy
Copy link
Member

timholy commented Dec 2, 2014

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

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

@timholy
Copy link
Member

timholy commented Dec 2, 2014

Thanks for explaining.

@ivarne
Copy link
Member

ivarne commented Dec 2, 2014

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.

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.

ifloor, iceil, itrunc
8 participants