Skip to content
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

Merged
merged 26 commits into from
Nov 20, 2023
Merged

Integrate RISC-V support #477

merged 26 commits into from
Nov 20, 2023

Conversation

luhenry
Copy link
Contributor

@luhenry luhenry commented Nov 4, 2023

It integrates the following changes:

@luhenry
Copy link
Contributor Author

luhenry commented Nov 4, 2023

This PR depends on #476

cc @blapie

Copy link
Collaborator

@blapie blapie left a 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 ?

@blapie
Copy link
Collaborator

blapie commented Nov 6, 2023

Hi!
Could you please put a link to passing GHA workflow?
Is there a chance this can be tested with a clang/llvm toolchain as well? SLEEF did not tend to merge features if not supported in both?

@luhenry
Copy link
Contributor Author

luhenry commented Nov 6, 2023

What is the rationale for adding the prefix ?

The intrinsics' name changed upstream. It's purely to follow with the spec and not a design preference.

Could you please put a link to passing GHA workflow?

https://github.com/rivosinc/sleef/actions/runs/6754458819

Is there a chance this can be tested with a clang/llvm toolchain as well? SLEEF did not tend to merge features if not supported in both?

It currently only works with clang as gcc doesn't have support for the RISC-V RVV intrinsics just yet.

@blapie
Copy link
Collaborator

blapie commented Nov 9, 2023

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.

@blapie
Copy link
Collaborator

blapie commented Nov 9, 2023

@ericlove @GlassOfWhiskey Would you be able to review the extra changes on top of yours?
@luhenry Do you know someone else the community that could review?
There is only a few changes in essence (19efad8) so I'll check and merge by next week, assuming the CI is in.

@luhenry luhenry marked this pull request as ready for review November 10, 2023 15:16
@luhenry
Copy link
Contributor Author

luhenry commented Nov 10, 2023

@blapie I've rebased the PR on top of the initial CI.

@GlassOfWhiskey
Copy link
Contributor

Hi all,
I'll review this today or tomorrow. Sry for the late reply.

@GlassOfWhiskey
Copy link
Contributor

GlassOfWhiskey commented Nov 12, 2023

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.

@luhenry
Copy link
Contributor Author

luhenry commented Nov 13, 2023

I’ll also look into adding gcc + llvm compilation as both of them support the intrinsics. It’ll also improve testing on other platforms.

@luhenry
Copy link
Contributor Author

luhenry commented Nov 13, 2023

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 master), but worth looking into some follow up PRs.

@blapie @GlassOfWhiskey let me know what you think

@ericlove
Copy link
Contributor

@blapie this looks good to me. Thanks to @luhenry for updating to the new intrinsics format!

@luhenry
Copy link
Contributor Author

luhenry commented Nov 13, 2023

I've enabled the LLVM builds for s390x and ppc64el. The issue was with cross-compilation of the builtin headers.

@GlassOfWhiskey
Copy link
Contributor

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 master), but worth looking into some follow up PRs.

@blapie @GlassOfWhiskey let me know what you think

I see that riscv builds with gcc are commented in the .github/workflow pipeline. Did they fail to compile for some reason?

@luhenry
Copy link
Contributor Author

luhenry commented Nov 13, 2023

I see that riscv builds with gcc are commented in the .github/workflow pipeline. Did they fail to compile for some reason?

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:

Clang 17 and GCC trunk supports the v0.12 version, no more incompatibility will be introduced.

@GlassOfWhiskey
Copy link
Contributor

@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 README.md that SLEEF now supports RISC-V V 1.0 with intrinsics >=0.12, just to make compatibility explicit.

@blapie
Copy link
Collaborator

blapie commented Nov 16, 2023

@blapie it should have everything now. Let me know what you think, thanks!

Will have one last pass today, but it looks good to me!

@luhenry
Copy link
Contributor Author

luhenry commented Nov 17, 2023

@blapie all tests are passing (including tester3) with the latest changes.

src/arch/helperrvv.h Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@luhenry
Copy link
Contributor Author

luhenry commented Nov 20, 2023

@blapie should be fixed.

@blapie
Copy link
Collaborator

blapie commented Nov 20, 2023

LGTM - Will merge now!

@blapie blapie merged commit cea4434 into shibatch:master Nov 20, 2023
27 checks passed
@blapie
Copy link
Collaborator

blapie commented Nov 20, 2023

Done. Thanks for a brilliant collab! I hope we can add support for the rest of the features soon.

@luhenry
Copy link
Contributor Author

luhenry commented Nov 20, 2023

Yes, I already have some ideas on what I want to improve for RISC-V. Thanks again!

joanaxcruz added a commit to joanaxcruz/sleef that referenced this pull request Jan 25, 2024
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.
blapie pushed a commit that referenced this pull request Jan 25, 2024
PR #477 pointed out some test failures with qemu.
Re-enabling these tests in GHA and testing locally
on x86 does not show errors.
@luhenry luhenry deleted the upstream-riscv branch March 25, 2024 07:59
mga-sc added a commit to mga-sc/llvm-project that referenced this pull request Oct 29, 2024
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.
mga-sc added a commit to mga-sc/llvm-project that referenced this pull request Oct 29, 2024
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.
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.

4 participants