Skip to content

Fix: expose std.math.ldexp as C ldexp, scalbn and scalbln in compiler-rt to address LLVM release-mode simplification #24142

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

th3james
Copy link

Updated version of #24006

Fixes #23358

Changes:

Impact:

  • Standard Zig implementation of ldexp is used everywhere. Assembly optimised implementations are lost.

Comment on lines +12 to +16
pub fn scalbln(x: f64, n: c_long) callconv(.c) f64 {
// mirror musl implementation - clamp c_long to i32
const clamped_n: i32 = @intCast(math.clamp(n, math.minInt(i32), math.maxInt(i32)));
return math.ldexp(x, clamped_n);
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this clamping behavior part of the C standard? We need to be careful about whether it should apply regardless of target.

@alexrp alexrp self-assigned this Jun 10, 2025
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Thanks @th3james.

Before this is merged:

  1. the logic for these functions need to move from std into compiler_rt
  2. new builtins need to be added for the functions
  3. the std code needs to be deleted in favor of calling those builtin functions
  4. new Legalize features added for expanding these builtin calls into compiler_rt calls
  5. llvm backend disables them and emits intrinsic calls
  6. other backends enable them

This is a pretty big scope increase, so you can hand it off to a core team member at any time.

@th3james
Copy link
Author

Happy to hand this off to someone else, as a first issue this was already pushing the limits of my expertise and free time. I’ll leave my branch live in case it’s a useful point of reference, feel free to close this PR or convert it to an issue/draft as you see fit.
Thank you both for taking the time to review, I learned a tonne.

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.

wasm-ld: ....o: undefined symbol: ldexp in Release modes, but works in Debug
3 participants