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.

Sign up for GitHub

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

Jump to bottom

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 •
edited
Loading

This PR depends on #476

cc @blapie

blapie
blapie reviewed Nov 6, 2023
View reviewed changes
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 •
edited
Loading

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 •
edited
Loading

@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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

@luhenry
Copy link
Contributor Author

luhenry commented Nov 17, 2023

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

ericlove
ericlove reviewed Nov 17, 2023
View reviewed changes
src/arch/helperrvv.h Outdated Show resolved Hide resolved
blapie
blapie reviewed Nov 20, 2023
View reviewed changes
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 mentioned this pull request Nov 20, 2023
Closed
@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!

This was referenced Nov 21, 2023
Closed
Closed
@luhenry luhenry mentioned this pull request Dec 11, 2023
Closed
3 tasks
joanaxcruz added a commit to joanaxcruz/sleef that referenced this pull request Jan 25, 2024
@joanaxcruz
Enable aarch64 llvm-17 cross compilation tests
b76489c 
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.
@joanaxcruz joanaxcruz mentioned this pull request Jan 25, 2024
Merged
blapie pushed a commit that referenced this pull request Jan 25, 2024
@joanaxcruz
Enable aarch64 llvm-17 cross compilation tests (#506)
d71aae0 
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
@Hamlin-Li Hamlin-Li mentioned this pull request Apr 10, 2024
Closed
3 tasks
@bedroge bedroge mentioned this pull request Jun 11, 2024
Open
mga-sc added a commit to mga-sc/llvm-project that referenced this pull request Oct 29, 2024
@mga-sc
[RISCV][SLEEF]: Support SLEEF vector library for RISC-V target.
529fde0 
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 mga-sc mentioned this pull request Oct 29, 2024
Merged
mga-sc added a commit to mga-sc/llvm-project that referenced this pull request Oct 29, 2024
@mga-sc
[RISCV][SLEEF]: Support SLEEF vector library for RISC-V target.
cb999c1 
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 Nov 7, 2024
@mga-sc
[RISCV][SLEEF]: Support SLEEF vector library for RISC-V target.
2df10bd 
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 Nov 12, 2024
@mga-sc
[RISCV][SLEEF]: Support SLEEF vector library for RISC-V target.
5ec5bac 
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.
asi-sc pushed a commit to llvm/llvm-project that referenced this pull request Nov 26, 2024
@mga-sc
[RISCV][SLEEF]: Support SLEEF vector library for RISC-V target. (#114014
93caee1 
)

SLEEF math vector library now supports 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
Reviewers

@blapie blapie blapie left review comments

@ericlove ericlove ericlove left review comments

@GlassOfWhiskey GlassOfWhiskey GlassOfWhiskey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@luhenry @blapie @GlassOfWhiskey @ericlove