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

fix fdlibm to use unions instead of unsafe pointer casts #46

Closed
zingales opened this issue Jun 14, 2011 · 25 comments
Closed

fix fdlibm to use unions instead of unsafe pointer casts #46

zingales opened this issue Jun 14, 2011 · 25 comments
Labels
bug Indicates an unexpected problem or unintended behavior

Comments

@zingales
Copy link
Contributor

When trying to make julia, test/unittests.j fails.

with the following error: Assertion failed: isinf(-(Inf))==true ./test/unittests.j:87

error in context:
make[1]: Leaving directory `/home/g3/julia/ui'
ln -f julia-release-readline julia
./julia ./test/unittests.j
Assertion failed: isinf(-(Inf))==true ./test/unittests.j:87
make: *** [release] Error 1

system info:
g3@ubuntu:~/julia$ uname -a
Linux ubuntu 2.6.38-8-generic 42-Ubuntu SMP Mon Apr 11 03:31:24 UTC 2011 x86_64 x86_64 x86_64 GNU/Linux

@JeffBezanson
Copy link
Sponsor Member

OK, good, I can reproduce this on a 32-bit csail machine.

@JeffBezanson
Copy link
Sponsor Member

Caused by building fdlibm with -O2 instead of -O1. It's officially time to fix the invalid pointer casts in fdlibm.
See fdlibm/math_private.h

@StefanKarpinski
Copy link
Sponsor Member

Or use crlibm.

@JeffBezanson
Copy link
Sponsor Member

Yeah, maybe this is a good opportunity to try crlibm and/or the assembly language versions.

@ViralBShah
Copy link
Member

Does anybody else use crlibm? Is it maintained?

@StefanKarpinski
Copy link
Sponsor Member

I have no idea if it's used, I'm afraid. I'm kind of shocked by the state of libm implementations in general — there just doesn't seem to be any single really good one. Although that pretty much reflects the general state of numerical computing as a whole, to be honest. We might need to pull together pieces of various implementations to stitch together something that's really as good as possible. Seems like a lot of annoying ancillary work.

@ViralBShah
Copy link
Member

Would be a good advertisement for julia if everyone came to our website for openlibm. :-)

-viral

On Jun 15, 2011, at 10:46 AM, StefanKarpinski wrote:

I have no idea if it's used, I'm afraid. I'm kind of shocked by the state of libm implementations in general — there just doesn't seem to be any single really good one. Although that pretty much reflects the general state of numerical computing in general, to be honest. We might need to pull together pieces of various implementations to stitch together something that's really as good as possible. Seems like a lot of annoying ancillary work.

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

@StefanKarpinski
Copy link
Sponsor Member

Yeah, that's really not a bad idea, actually.

@JeffBezanson
Copy link
Sponsor Member

Can we divvy up the files and fix this? There are 80 files to fix, about 26 each, and some of the files might not need changes, so it's not that bad.

@StefanKarpinski
Copy link
Sponsor Member

There are actually only 49 files that use the __LO and __HI macros. Here are the files, randomly divided into three groups, each assigned to one of us.

Jeff:

e_asin.c
e_cosh.c
e_hypot.c
e_pow.c
e_remainder.c
fdlibm.h
k_tan.c
s_atan.c
s_expm1.c
s_fabs.c
s_finite.c
s_frexp.c
s_logb.c
s_modf.c
s_nextafter.c
s_sin.c

Stefan:

e_acosh.c
e_atanh.c
e_fmod.c
e_j0.c
e_j1.c
e_jn.c
e_lgamma_r.c
e_log.c
e_log10.c
e_rem_pio2.c
k_cos.c
s_asinh.c
s_cbrt.c
s_floor.c
s_ilogb.c
s_isnan.c
s_tan.c

Viral:

e_acos.c
e_atan2.c
e_exp.c
e_log2.c
e_sinh.c
e_sqrt.c
k_sin.c
k_standard.c
s_ceil.c
s_copysign.c
s_cos.c
s_erf.c
s_log1p.c
s_rint.c
s_scalbn.c
s_tanh.c

Yes, I used Julia to do the dividing :-)

@JeffBezanson
Copy link
Sponsor Member

Should we all send you patches so it goes in as a single commit?

@ViralBShah
Copy link
Member

Can't the whole thing be done with sed or awk?

On 16-Jun-2011, at 3:44 AM, [email protected] wrote:

There are actually only 49 files that actually use the __LO and __HI macros. Here are the files, randomly divided into three groups, each assigned to one of us.

Jeff:

e_asin.c
e_cosh.c
e_hypot.c
e_pow.c
e_remainder.c
fdlibm.h
k_tan.c
s_atan.c
s_expm1.c
s_fabs.c
s_finite.c
s_frexp.c
s_logb.c
s_modf.c
s_nextafter.c
s_sin.c

Stefan:

e_acosh.c
e_atanh.c
e_fmod.c
e_j0.c
e_j1.c
e_jn.c
e_lgamma_r.c
e_log.c
e_log10.c
e_rem_pio2.c
k_cos.c
s_asinh.c
s_cbrt.c
s_floor.c
s_ilogb.c
s_isnan.c
s_tan.c

Viral:

e_acos.c
e_atan2.c
e_exp.c
e_log2.c
e_sinh.c
e_sqrt.c
k_sin.c
k_standard.c
s_ceil.c
s_copysign.c
s_cos.c
s_erf.c
s_log1p.c
s_rint.c
s_scalbn.c
s_tanh.c

Yes, I used Julia to do the dividing :-)

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

@JeffBezanson
Copy link
Sponsor Member

Not in cases like if (__LO(x)...) where you need to introduce a temporary variable.

@JeffBezanson
Copy link
Sponsor Member

You can give me some of your functions if you want; I'm done with mine (I have no life outside julia).

@ViralBShah
Copy link
Member

I won't be able to touch code for the next 2-3 days - unfortunately. So, feel free to go down my list. Also, when I have time, I would like to focus on parallel HPL. Sorry about this one.

-viral

On Jun 16, 2011, at 7:35 AM, JeffBezanson wrote:

You can give me some of your functions if you want; I'm done with mine (I have no life outside julia).

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

@StefanKarpinski
Copy link
Sponsor Member

Re single vs. multiple patches. I don't think it matters. You can send me patches using gist. On the other hand, these can be fixed incrementally, so there's no real harm in multiple commits. I wouldn't mind seeing how you did yours as a guide for cases I may not be able to figure out. I haven't gotten a chance to tackle yet...

@JeffBezanson
Copy link
Sponsor Member

OK, pushed (commit b248040). I will take Viral's functions.

@StefanKarpinski
Copy link
Sponsor Member

Sorry I didn't get a chance to do my functions before you did them. Thanks, Jeff.

@StefanKarpinski
Copy link
Sponsor Member

Oh, I thought you did them all — now I see that you just did more and that there are many usages of __LO and __HI still left.

@JeffBezanson
Copy link
Sponsor Member

fixed the rest in commit 414ac2f

@ViralBShah
Copy link
Member

Hmm, maybe native is a new gcc switch? Also, the trouble with native tuning is that you have to have a way to compile in code for all families for distribution. Cloud seems to get even more appealing for these kinds of reasons.

gcc -D_IEEE_LIBM -Dx86 -fPIC -O2 -march=native -c -o k_cos.o k_cos.c
k_cos.c:1: error: bad value (native) for -march= switch
k_cos.c:1: error: bad value (native) for -mtune= switch
make[2]: *** [k_cos.o] Error 1
make[1]: *** [fdlibm/libfdm.dylib] Error 2
make: *** [julia-release] Error 2

-viral

On Jun 17, 2011, at 1:43 AM, JeffBezanson wrote:

fixed the rest in commit 414ac2f

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

@ViralBShah
Copy link
Member

Its odd I get that error, even when native is discussed in my gcc manpage.

-viral

On Jun 17, 2011, at 1:45 AM, Viral Shah wrote:

Hmm, maybe native is a new gcc switch? Also, the trouble with native tuning is that you have to have a way to compile in code for all families for distribution. Cloud seems to get even more appealing for these kinds of reasons.

gcc -D_IEEE_LIBM -Dx86 -fPIC -O2 -march=native -c -o k_cos.o k_cos.c
k_cos.c:1: error: bad value (native) for -march= switch
k_cos.c:1: error: bad value (native) for -mtune= switch
make[2]: *** [k_cos.o] Error 1
make[1]: *** [fdlibm/libfdm.dylib] Error 2
make: *** [julia-release] Error 2

-viral

On Jun 17, 2011, at 1:43 AM, JeffBezanson wrote:

fixed the rest in commit 414ac2f

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

@ViralBShah
Copy link
Member

sqrt() seems to run about 20-30% faster than before, if memory serves correctly.

-viral

On Jun 17, 2011, at 1:45 AM, Viral Shah wrote:

Hmm, maybe native is a new gcc switch? Also, the trouble with native tuning is that you have to have a way to compile in code for all families for distribution. Cloud seems to get even more appealing for these kinds of reasons.

gcc -D_IEEE_LIBM -Dx86 -fPIC -O2 -march=native -c -o k_cos.o k_cos.c
k_cos.c:1: error: bad value (native) for -march= switch
k_cos.c:1: error: bad value (native) for -mtune= switch
make[2]: *** [k_cos.o] Error 1
make[1]: *** [fdlibm/libfdm.dylib] Error 2
make: *** [julia-release] Error 2

-viral

On Jun 17, 2011, at 1:43 AM, JeffBezanson wrote:

fixed the rest in commit 414ac2f

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

@JeffBezanson
Copy link
Sponsor Member

We still need to see if the assembly language sqrt is accurate enough. The
Intel manual doesn't say anything about it.

On Thu, Jun 16, 2011 at 4:24 PM, ViralBShah <
[email protected]>wrote:

sqrt() seems to run about 20-30% faster than before, if memory serves
correctly.

-viral

On Jun 17, 2011, at 1:45 AM, Viral Shah wrote:

Hmm, maybe native is a new gcc switch? Also, the trouble with native
tuning is that you have to have a way to compile in code for all families
for distribution. Cloud seems to get even more appealing for these kinds of
reasons.

gcc -D_IEEE_LIBM -Dx86 -fPIC -O2 -march=native -c -o k_cos.o k_cos.c
k_cos.c:1: error: bad value (native) for -march= switch
k_cos.c:1: error: bad value (native) for -mtune= switch
make[2]: *** [k_cos.o] Error 1
make[1]: *** [fdlibm/libfdm.dylib] Error 2
make: *** [julia-release] Error 2

-viral

On Jun 17, 2011, at 1:43 AM, JeffBezanson wrote:

fixed the rest in commit 414ac2f

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

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

@ViralBShah
Copy link
Member

Ok. I meant the fdlibm sqrt with -O2 is faster than before.

-viral

On Jun 17, 2011, at 1:56 AM, JeffBezanson wrote:

We still need to see if the assembly language sqrt is accurate enough. The
Intel manual doesn't say anything about it.

On Thu, Jun 16, 2011 at 4:24 PM, ViralBShah <
[email protected]>wrote:

sqrt() seems to run about 20-30% faster than before, if memory serves
correctly.

-viral

On Jun 17, 2011, at 1:45 AM, Viral Shah wrote:

Hmm, maybe native is a new gcc switch? Also, the trouble with native
tuning is that you have to have a way to compile in code for all families
for distribution. Cloud seems to get even more appealing for these kinds of
reasons.

gcc -D_IEEE_LIBM -Dx86 -fPIC -O2 -march=native -c -o k_cos.o k_cos.c
k_cos.c:1: error: bad value (native) for -march= switch
k_cos.c:1: error: bad value (native) for -mtune= switch
make[2]: *** [k_cos.o] Error 1
make[1]: *** [fdlibm/libfdm.dylib] Error 2
make: *** [julia-release] Error 2

-viral

On Jun 17, 2011, at 1:43 AM, JeffBezanson wrote:

fixed the rest in commit 414ac2f

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

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

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

vtjnash added a commit that referenced this issue Jan 8, 2013
…m synchronous methods, fix web_repl (by replacing code with original from JuliaLang).

closes #50. closes #46. closes #40 (i think). closes #25.
StefanKarpinski pushed a commit that referenced this issue Feb 8, 2018
cmcaine pushed a commit to cmcaine/julia that referenced this issue Sep 24, 2020
cmcaine pushed a commit to cmcaine/julia that referenced this issue Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

4 participants