-
Notifications
You must be signed in to change notification settings - Fork 87
Use platform intrinsics, not LLVM, for floor & ceil #47
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
Conversation
This reverts commit 3ad356d.
There are no platform intrinsics in the rustc for these functions yet, so this removes them as a distinct commit for later reversion.
This is blocked on rust-lang/rust#79560, indeed the entire library might be. |
3d9bbf9
to
e9cc306
Compare
This is no longer blocked. Please disregard Travis' "pull request' failure, it was based on the spurious error from before. |
Huh, there is a further issue but only with the mask types on wasm SIMD. I assume this was not introduced by this patch, so there should be no reason not to merge this. big fat error logfailures:---- ops::ops_impl::mask128::mask128x4::bitand output ----
---- ops::ops_impl::mask128::mask128x4::bitand_assign output ----
---- ops::ops_impl::mask128::mask128x4::bitor output ----
---- ops::ops_impl::mask128::mask128x4::bitor_assign output ----
---- ops::ops_impl::mask128::mask128x4::bitxor output ----
---- ops::ops_impl::mask128::mask128x4::bitxor_assign output ----
failures:
test result: FAILED. 2212 passed; 6 failed; 0 ignored console.log div contained:
|
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.
Nice! 🎉
I didn't realise we already had intrinsics for rounding. Were those just added or is there some automagic handling of the simd_{op}
name that lines up with the right LLVM intrinsic? (I already forget how that works...)
We do not have the full set of trunc or round platform intrinsics (and so lose that functionality here), but they can be added to rustc. I wanted to make sure that this would fix that before I moved on to prioritizing that. |
To actually answer the question: it is indeed the case that the compiler essentially handles platform intrinsics in LLVM codegen by performing an automagic translation. This is likely why it works for riscv64gc... I suspect there is something about how the intrinsics were specified in-lib that was preempting LLVM's codegen logic. |
Yup, we were using |
There is |
Should this get merged in we should update #38 with the removed functions. |
@bjorn3 Wouldn't that still cause issues on cranelift's end? |
Yes |
This PR removes the direct linkage to LLVM for trunc and round intrinsics, while replacing that link with rustc's platform intrinsics for floor and ceil functions, namely
simd_floor
andsimd_ceil
. Tests that are no longer testable are removed. In doing so it resolves the riscv64gc compilation problems.This PR will close #35.