search

shibatch / sleef

SIMD Library for Evaluating Elementary Functions, vectorized libm and DFT
https://sleef.org
Boost Software License 1.0
661 stars 131 forks source link

Integrate RISC-V support #477

Closed luhenry closed 11 months ago

luhenry commented 11 months ago

It integrates the following changes:

luhenry commented 11 months ago

This PR depends on https://github.com/shibatch/sleef/pull/476

cc @blapie

blapie commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

@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 (https://github.com/shibatch/sleef/pull/477/commits/19efad8e19259dda7aaca7882523534912bb23ec) so I'll check and merge by next week, assuming the CI is in.

luhenry commented 11 months ago

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

GlassOfWhiskey commented 11 months ago

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

GlassOfWhiskey commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

luhenry commented 11 months ago

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

GlassOfWhiskey commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

@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.

GlassOfWhiskey commented 11 months ago

Sorry Isee that README is quite minimal and doesn't say anything about compatibility. We can put this in the CHANGELOG.md before next release

luhenry commented 11 months ago

aarch64, ppc64el, and s390x fail with GCC 12 with the following error:

tanf denormal/nonnumber test : arg = -0, test = 0, correct = -0

The error isn't there when compiling with GCC 11, so downgrading to that and I'll let you explore why that is.

blapie commented 11 months ago

aarch64, ppc64el, and s390x fail with GCC 12 with the following error:

tanf denormal/nonnumber test : arg = -0, test = 0, correct = -0

The error isn't there when compiling with GCC 11, so downgrading to that and I'll let you explore why that is.

Thank you! We also observed this. Created #483 to track this.

luhenry commented 11 months ago

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

blapie commented 11 months ago

@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 commented 11 months ago

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

luhenry commented 11 months ago

@blapie should be fixed.

blapie commented 11 months ago

LGTM - Will merge now!

blapie commented 11 months ago

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

luhenry commented 11 months ago

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