-
Notifications
You must be signed in to change notification settings - Fork 131
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
Integrate RISC-V support #477
Conversation
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.
What is the rationale for adding the prefix ?
Hi! |
The intrinsics' name changed upstream. It's purely to follow with the spec and not a design preference.
https://github.com/rivosinc/sleef/actions/runs/6754458819
It currently only works with clang as gcc doesn't have support for the RISC-V RVV intrinsics just yet. |
Got you. I understand that it might take some time to be supported in gcc. It would be a shame not to make this available though. |
@ericlove @GlassOfWhiskey Would you be able to review the extra changes on top of yours? |
b57a01a
to
dac4bba
Compare
- intrinsic functions are now prefixed with __riscv_ - vmerge/vfmerge argument order has changed
dac4bba
to
445906b
Compare
445906b
to
55fd453
Compare
@blapie I've rebased the PR on top of the initial CI. |
Hi all, |
As a first comment, since the RISC-V vector intrinsics standard has not reached a stable version yet, it could be useful to document which version of the intrinsics are we using right now. |
I’ll also look into adding gcc + llvm compilation as both of them support the intrinsics. It’ll also improve testing on other platforms. |
I've added the gcc and llvm builds, but I'm getting quite a few failures. I've explicitly disabled the configurations that don't work (no regression compared to @blapie @GlassOfWhiskey let me know what you think |
I've enabled the LLVM builds for s390x and ppc64el. The issue was with cross-compilation of the builtin headers. |
I see that |
Yes, the gcc I'm getting from riscv-gnu-toolchain isn't recent enough, it needs to be GCC trunk and it's only GCC 13.2 at the moment. Once there is a new GCC release, we should be able to bump that version at .github/workflows/build_and_test.yml#L145 and be able to test with GCC. Per the documentation of the RISC-V V intrinsics, LLVM-17 should have the final version of the intrinsics, so building and testing with that is the best we have today. See https://github.com/riscv-non-isa/rvv-intrinsic-doc/blob/main/README.md:
|
@luhenry it makes sense to me. For me the PR is ready to be merged. Maybe as a last bit, you could put in the |
Will have one last pass today, but it looks good to me! |
@blapie all tests are passing (including tester3) with the latest changes. |
@blapie should be fixed. |
LGTM - Will merge now! |
Done. Thanks for a brilliant collab! I hope we can add support for the rest of the features soon. |
Yes, I already have some ideas on what I want to improve for RISC-V. Thanks again! |
PR shibatch#477 pointed out some test failures with qemu. However, following the discussion on this PR cannot see exactly what these failures were (none of the GHA past builds show runs for these settings). Locally, cross compiling aarch64 on x86 host with llvm-17 and using user-mode qemu emulation environment do not show failures. Therefore we try to enable these tests in GHA and check for failures.
PR #477 pointed out some test failures with qemu. Re-enabling these tests in GHA and testing locally on x86 does not show errors.
SLEEF math vector library support RISC-V target. Commit: shibatch/sleef#477 This patch enables the use of auto-vectorization with subsequent replacement by the corresponding SLEEF function.
SLEEF math vector library support RISC-V target. Commit: shibatch/sleef#477 This patch enables the use of auto-vectorization with subsequent replacement by the corresponding SLEEF function.
It integrates the following changes: