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.

Sign up for GitHub

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

Jump to bottom

make sure Julia doesn't use x87 math. #43978

Merged
merged 13 commits into from
Feb 8, 2022
Merged

make sure Julia doesn't use x87 math. #43978

merged 13 commits into from
Feb 8, 2022

Conversation

simeonschaub
Copy link
Member

From discussion with @Keno on slack, I believe this is the reasons the
tests added in #43870 are failing on linux32. (As seen in
https://build.julialang.org/#/builders/17/builds/1356/steps/5/logs/stdio
for example.)

Not sure if RT_LIBS is where this should go, so I'd appreciate any
feedback.

@simeonschaub simeonschaub added bugfix This change fixes an existing bug building Build system, or building Julia or its dependencies labels Jan 29, 2022
@simeonschaub simeonschaub requested a review from Keno January 29, 2022 03:10
Keno
Keno reviewed Jan 29, 2022
View reviewed changes
src/Makefile Outdated Show resolved Hide resolved
@simeonschaub
properly link libjulia to openlibm
f2e84c7 
From discussion with @Keno on slack, I believe this is the reasons the
tests added in #43870 are failing on linux32. (As seen in
https://build.julialang.org/#/builders/17/builds/1356/steps/5/logs/stdio
for example.)

Not sure if `RT_LIBS` is where this should go, so I'd appreciate any
feedback.
@DilumAluthge DilumAluthge requested a review from Keno January 30, 2022 14:47
@simeonschaub simeonschaub reopened this Jan 30, 2022
@DilumAluthge
Copy link
Member

DilumAluthge commented Jan 30, 2022 •
edited
Loading

@Keno Is this good to merge once CI is green?

@simeonschaub
Copy link
Member Author

Hmm, the linux32 binaries don't seem to explicitly link against glibc anymore, but the test is still failing:

$ nm julia-f2e84c728d/lib/julia/libjulia-internal.so | rg sqrt
000963ce T jl_sqrt_llvm
00096614 T jl_sqrt_llvm_fast
000963fb T jl_sqrt_llvm_fast_withtype
000961b5 T jl_sqrt_llvm_withtype
         U sqrt
         U sqrtf
00140890 t _ZNK4llvm5APInt4sqrtEv
00140890 t _ZNK4llvm5APInt4sqrtEv.localalias.38
002b64a0 r _ZZNK4llvm5APInt4sqrtEvE19__PRETTY_FUNCTION__
002b6fa0 r _ZZNK4llvm5APInt4sqrtEvE7results

@Keno
Copy link
Member

Keno commented Jan 31, 2022

The issue is that the link line ends up being -lm -lz -lopenlibm. I guess LLVM adds -lm to the link line, which is bad, since that should be openlibm, but maybe for now, just add a second copy of the $(LIBM) before LLVM to fix that.

simeonschaub
simeonschaub commented Jan 31, 2022
View reviewed changes
src/Makefile Outdated Show resolved Hide resolved
@Keno
Copy link
Member

Keno commented Jan 31, 2022

Doesn't look like that worked either. Let's just statically link openlibm. There's really no point to making it a dynamic library and it just opens us open to all kinds of dynamic linker shenanigans.

@oscardssmith
Copy link
Member

I think the problem is --system-libs on line 139. If remove it, the -lm doesn't get added. (we may need to manually add the system libs we actually want though)

@oscardssmith
Copy link
Member

It looks like this works!

@DilumAluthge
Copy link
Member

It looks like tester_linux32 hasn't started yet.

@oscardssmith
Copy link
Member

oops. I looked at the package rather than the tester.

@DilumAluthge DilumAluthge reopened this Feb 3, 2022
Keno
Keno reviewed Feb 3, 2022
View reviewed changes
src/Makefile Outdated Show resolved Hide resolved
@oscardssmith
Copy link
Member

Why isn't tester_linux_32 running?

@simeonschaub
Copy link
Member Author

@DilumAluthge Could you clear the cue again?

@Keno
Copy link
Member

Keno commented Feb 4, 2022

If you need to do more debugging after this, maybe just run the docker container locally. Get yourself some millisecond cycle time rather than hours.

@simeonschaub simeonschaub mentioned this pull request Feb 4, 2022
Closed
@oscardssmith
Copy link
Member

It turns out the problem wasn't the linking between Julia and libm, but just that we were compiling runtime-intrisics.c with settings that allowed it to do math in 80 bit precision which breaks stuff.

vtjnash
vtjnash reviewed Feb 8, 2022
View reviewed changes
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

I don't seem to see any code changes resulting from this in jl_sqrt_llvm_withtype

Make.inc Outdated Show resolved Hide resolved
@Keno
Copy link
Member

Keno commented Feb 8, 2022

I don't seem to see any code changes resulting from this in jl_sqrt_llvm_withtype

Are you building 32bit linux?

@DilumAluthge
Copy link
Member

Also, analyzegc is sad now.

@Keno
Copy link
Member

Keno commented Feb 8, 2022

Also, analyzegc is sad now.

Clang doesn't understand the option, so needs to either be made GCC specific, add -Wno-ignored-optimization-argument, or see if @vtjnash's suggestion above fixes it instead.

Keno
Keno reviewed Feb 8, 2022
View reviewed changes
Make.inc Outdated Show resolved Hide resolved
Keno
Keno reviewed Feb 8, 2022
View reviewed changes
Make.inc Outdated Show resolved Hide resolved
Keno
Keno reviewed Feb 8, 2022
View reviewed changes
Make.inc Outdated Show resolved Hide resolved
@oscardssmith
Copy link
Member

Ready to merge? (Merging tomorrow sans objection)

@simeonschaub
Copy link
Member Author

Should probably change the name, but thanks for figuring this out!

@oscardssmith oscardssmith changed the title properly link libjulia to openlibm make sure Julia doesn't use x87 math. Feb 8, 2022
@oscardssmith oscardssmith merged commit 942697f into master Feb 8, 2022
@oscardssmith oscardssmith deleted the sds/link_openlibm branch February 8, 2022 18:11
JeffBezanson
JeffBezanson reviewed Feb 8, 2022
View reviewed changes
Make.inc
@@ -874,6 +874,10 @@ ifneq (,$(filter $(ARCH), powerpc64le ppc64le))
DIST_ARCH:=ppc64le
endif
ifeq (1,$(ISX86))
# on x86 make sure not to use 80 bit math when we want 64 bit math.
ifeq (32,$BINARY))
Copy link
Member

Choose a reason for hiding this comment

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

missing a ( here

Copy link
Member

Choose a reason for hiding this comment

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

#44081

@KristofferC
Copy link
Member

x87 math? ;)

@Keno
Copy link
Member

Keno commented Feb 8, 2022

x87 math? ;)

yes (https://en.wikipedia.org/wiki/X87)

@oscardssmith oscardssmith mentioned this pull request Feb 8, 2022
Merged
vchuravy added a commit that referenced this pull request Feb 8, 2022
@vchuravy
Merge pull request #44081 from oscardssmith/fix-linux32-ci
60f414e 
fix #43978 (missing parens)
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request Feb 17, 2022
@simeonschaub @oscardssmith @antoine-levitt
make sure Julia doesn't use x87 math. (JuliaLang#43978)
9c394b7 
* use fpmath=sse on 32 bit x86 to prevent 80 bit floats causing double rounding

Co-authored-by: Oscar Smith <oscardssmith@gmail.com>
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
@simeonschaub @oscardssmith @LilithHafner
make sure Julia doesn't use x87 math. (JuliaLang#43978)
da79506 
* use fpmath=sse on 32 bit x86 to prevent 80 bit floats causing double rounding

Co-authored-by: Oscar Smith <oscardssmith@gmail.com>
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
@simeonschaub @oscardssmith @LilithHafner
make sure Julia doesn't use x87 math. (JuliaLang#43978)
dd476fd 
* use fpmath=sse on 32 bit x86 to prevent 80 bit floats causing double rounding

Co-authored-by: Oscar Smith <oscardssmith@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oscardssmith oscardssmith oscardssmith approved these changes

@Keno Keno Keno approved these changes

@JeffBezanson JeffBezanson JeffBezanson left review comments

@vtjnash vtjnash vtjnash left review comments

Assignees
No one assigned
Labels
bugfix This change fixes an existing bug building Build system, or building Julia or its dependencies
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@simeonschaub @DilumAluthge @Keno @oscardssmith @KristofferC @vtjnash @JeffBezanson