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

Add openlibm to sysimg link line on windows #53672

Merged
merged 4 commits into from
Jun 18, 2024
Merged

Conversation

Keno
Copy link
Member

@Keno Keno commented Mar 9, 2024

LLVM generates calls to math intrinsics like trunc and rint (at least in my local i686 mingw) build, so linking to openlibm is required. We already have this on the sysimg link line in Base.link_image, so this aligns those options.

LLVM generates calls to math intrinsics like `trunc` and `rint` (at least in my local i686 mingw) build, so linking to openlibm is required. We already have this on the sysimg link line in `Base.link_image`, so this aligns those options.
Copy link
Sponsor 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 thought this was supposed to come from Make.inc (OSLIBS or some variable specific to openlibm--I forget which) since the user might have wanted to override the default build options for it

@Keno
Copy link
Member Author

Keno commented Mar 9, 2024

We define $(LIBM). I can use that here, but we don't really use it anywhere else (although we do use $(LIBMNAME) which julia picks up for all explicit (not LLVM-generated) libm calls). Should I replace this by $(LIBM). I think we may also want to make it unconditional. On non-Windows platforms it's probably picking up the implicit system libm, which is undesirable.

@ViralBShah
Copy link
Member

I suspect modern libms on most 64-bit platforms are actually better than openlibm.

@PallHaraldsson
Copy link
Contributor

I thought we (almost) got rid of [open]libm dependency, i.e. could for all platforms except win32. I.e. for Julia itself. Does this mean it's though needed for LLVM, and only then? And only for trunc and rint, and maybe very few other?

Does this mean dynamically linking to openlib, then bundling/bloating with it, or statically, and only few functions could (theoretically) be provided, i.e. a cut-down libm, if the compiler doesn't actually take care of dead-code-eliminating?

What modern libms did you have in mind (for Windows)?

On non-Windows platforms it's probably picking up the implicit system libm, which is undesirable.

Why? Is it about possibly non-existent or old/inferior? On Windows at least, isn't it always there (and actually part of libc, as on Android?). And always there on macOS, and all Linux/Unix-like sane platforms? And at least those functions available, non-buggy, and fast enough?

I see what's provided:
double trunc(double x);
float truncf(float x);
long double truncl(long double x);

Only actually first (1 or) 2 needed, plus actually more for BigFloat (already covered elsewhere), though not for LLVM...

@ViralBShah ViralBShah added domain:building Build system, or building Julia or its dependencies system:windows Affects only Windows labels Mar 9, 2024
@ViralBShah
Copy link
Member

Should we merge for now?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jun 3, 2024

It seems good, just rather suspicious that we would hard code it like this, rather than use a Make variable such as $(LIBM)

@staticfloat staticfloat merged commit 320366b into master Jun 18, 2024
7 checks passed
@staticfloat staticfloat deleted the kf/sysimglinkopenlibm branch June 18, 2024 02:55
KristofferC pushed a commit that referenced this pull request Jun 24, 2024
LLVM generates calls to math intrinsics like `trunc` and `rint` (at
least in my local i686 mingw) build, so linking to openlibm is required.
We already have this on the sysimg link line in `Base.link_image`, so
this aligns those options.

---------

Co-authored-by: Elliot Saba <[email protected]>
(cherry picked from commit 320366b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:building Build system, or building Julia or its dependencies system:windows Affects only Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants