Skip to content

runtime_intrinsics.c: Correct max_double #57124

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

Merged
merged 5 commits into from
Jan 22, 2025

Conversation

eschnett
Copy link
Contributor

Closes #57119.

@adienes
Copy link
Member

adienes commented Jan 22, 2025

added tests would be prudent 🙂 (although I know you were not the pr original author)

@oscardssmith oscardssmith added embarrassing-bugfix Whoops! compiler:codegen Generation of LLVM IR and native code bugfix This change fixes an existing bug labels Jan 22, 2025
@oscardssmith
Copy link
Member

A test would be good, but this also seems to me like a really good reason for us to move away from runtime intrinsics. They're much more possible to break without people noticing.

@giordano giordano added the needs tests Unit tests are required for this change label Jan 22, 2025
@giordano
Copy link
Contributor

I'm a bit confused by how this wasn't caught before. Regular uses of min/max work correctly, how are these intrinsics used?

@oscardssmith
Copy link
Member

So there are 2 implementations of all of our intrinsics, the codegen implementation (in codegen.c or cgutils.c) which is the LLVM code that we generate when compiling the function, and the runtime.c implementation that gets called when the function is interpreted.

@giordano
Copy link
Contributor

#57119 (comment) suggested tests and where to put them. For example, but not limited to:

julia> Core.Intrinsics.max_float(1.0, 2.0)
1.0

julia> Core.Intrinsics.max_float(1.0f0, 2.0f0)
2.0f0

@giordano giordano removed the needs tests Unit tests are required for this change label Jan 22, 2025
@JeffBezanson JeffBezanson merged commit 090e291 into JuliaLang:master Jan 22, 2025
7 of 8 checks passed
@eschnett eschnett deleted the patch-9 branch January 22, 2025 22:47
xal-0 pushed a commit to xal-0/julia that referenced this pull request Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug compiler:codegen Generation of LLVM IR and native code embarrassing-bugfix Whoops!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in max_double implementation
5 participants