-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 using roundeven intrinsic on darwin #10479
Conversation
For some reason the `roundeven` intrinsic is not available in LLVM 11 on macos, so we fall back to `rint`.
Thank you! Do you know why that function isn't there on Mac? And what's the benefit of using that over the alternative? |
I have no idea why it's missing on macos. But it should be available in LLVM 11 (https://releases.llvm.org/11.0.0/docs/LangRef.html#llvm-roundeven-intrinsic) and it does on linux. As far as I understand, |
Well, the docs say it's not supported on all target. Maybe this is one case. |
Could be. But when a major platform doesn't support even the main float types, I'd expect a more clear message that it's only supported on some targets at all. |
Is there a performance difference or something else? I think that if they are equivalent but one works in more platforms, we should use that alternative. |
Well Since we don't know about other platforms, I'd limit usage to LLVM >= 11 on linux. That should be good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since llvm@11 support still has it's gotchas (ref: #10359). I think this is fine as is. The official releases does not use llvm@11 and we can even fallback to llvm@9 in homebrew. Meanwhile this can be merged.
Worst-case, if roundeven_f32 is missing on some platform, the method can easily be monkey-patched and a PR submitted.
I actually think we should use |
@straight-shoota Thank you! And just to be clear, it might have looked like I'm totally against using |
@straight-shoota, thank you! It fixes compiler at least on Alpine and with latest LLVM versions. Compiler compiled with |
Resolves #10475
Based on #10476 to make sure it actually works (and continues to work).