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

Risc V and gcc-13 #579

Closed
atupone opened this issue Sep 20, 2024 · 12 comments · Fixed by #602
Closed

Risc V and gcc-13 #579

atupone opened this issue Sep 20, 2024 · 12 comments · Fixed by #602

Comments

@atupone
Copy link

atupone commented Sep 20, 2024

See
https://bugs.gentoo.org/939400

It seems that sleef (3.6.1), on Risc-V, check for SIMD instruction that belongs to the v0.11.x draft
but then during build uses instructions in the v1.0.

gcc-13 only support the v0.11.x draft

I suggest checking instruction set belonging to the v1.0 (e.g. vfloat64m1x4_t)

Unfortunately I don't have a risc-v hardware to test

@shibatch
Copy link
Owner

As for the RISC-V architecture, gcc-13 is not supported, see README.
I get so many errors when building with gcc-13. It is unlikely that gcc-13 will ever be supported.
I tried with gcc-14.2.0 and it passed the tests.

@orlitzky
Copy link
Contributor

Apparently it does build OK and pass tests once RVV is manually disabled -- we discovered this on the Gentoo bug.

I think all of the build failures are due to RVV being misdetected. With gcc-13, both RVV-1.0 and RVV-2.0 are detected but neither are implemented. This leads to build failures because sleef actually needs v1.0 to be fully implemented. The mis-detection with gcc-13 is due to the CMake tests that look for some types present in the v0.11.x draft of RVV. These are present in gcc-13 (so the test passes), but the v1.0 types aren't (so the build fails).

As @atupone suggests this can probably be fixed by checking for some types that are present in v1.0 and v2.0 only. I would be happy to test. I already have gcc-14 installed and it would be easy to install gcc-13 as well.

@blapie
Copy link
Collaborator

blapie commented Sep 25, 2024

@luhenry @GlassOfWhiskey @ericlove I thought I should draw your attention on this issue. Can you think of a way to make feature detection a bit more robust for RVV and avoid the mis-detection mentioned above for gcc 13+?

I will see what we can do regarding testing with gcc 13/14 in CI.

@blapie
Copy link
Collaborator

blapie commented Sep 25, 2024

@atupone @orlitzky Thanks for reporting the issue, feel free to suggest a fix. We will be able to test it locally with the appropriate compilers, although we also don't have access to hardware.

@ericlove
Copy link
Contributor

@luhenry @GlassOfWhiskey @ericlove I thought I should draw your attention on this issue. Can you think of a way to make feature detection a bit more robust for RVV and avoid the mis-detection mentioned above for gcc 13+?

The standard way of testing for RVV v1.0 ISA compatibility would be to compare the __riscv_v macro against 1000000, but in this case I think we should also compare the intrinsics API version against 12000 to ensure the tuple types are defined.

@blapie
Copy link
Collaborator

blapie commented Oct 28, 2024

Hi! Is someone able to give that a go? If I understand the suggestion is to use a different intrinsic/type in the detection mechanism, e.g. https://github.com/shibatch/sleef/blob/master/Configure.cmake#L664

Currently the code that's used for checking is

    vint32m2_t r = __riscv_vmv_v_x_i32m2(1, 2 * __riscv_vlenb() * 8 / 32); }"

I'm not familiar with RISC-V types and intrinsics, let alone across version of the architecture, what would be a good choice of type or intrisinc to avoid this misdetection?

@orlitzky
Copy link
Contributor

orlitzky commented Nov 1, 2024

I guess I will take a stab at it using @ericlove's suggestion

@blapie
Copy link
Collaborator

blapie commented Nov 1, 2024

Thank you @orlitzky!

@ericlove
Copy link
Contributor

ericlove commented Nov 1, 2024

Indeed, sorry to have dropped the ball on this, but thanks @orlitzky for taking it on, and I'd be happy to review. 👍

@orlitzky
Copy link
Contributor

orlitzky commented Nov 2, 2024

An RVV1 attempt here: #602

I have two questions now:

  1. I was able to find the spec for __riscv_v_intrinsic, but I couldn't find any documentation for __riscv_v. Both GCC and clang seem to know about it, but I'd be happier if I saw it written down somewhere. Anyone know?
  2. What should we do about RVV2? The default build still fails with GCC 13 because RVV2 support is detected. (I don't even know what RVV2 is referring to exactly.)

@ericlove
Copy link
Contributor

ericlove commented Nov 4, 2024

  • I was able to find the spec for __riscv_v_intrinsic, but I couldn't find any documentation for __riscv_v. Both GCC and clang seem to know about it, but I'd be happier if I saw it written down somewhere. Anyone know?
  • What should we do about RVV2? The default build still fails with GCC 13 because RVV2 support is detected. (I don't even know what RVV2 is referring to exactly.)

@orlitzky thanks for making #602 ! Regarding these questions:

  1. The __riscv_v macro is not part of the intrinsics specification, but is instead generated by the presence of the "V" extension in the ISA string of the compiler target. This is documented in the "Architecture Extension Test Macros" section of the RISC-V C API doc.
  2. The RVVM1 and RVVM2 targets refer to two different versions of the SLEEF RVV port, corresponding to the value of LMUL in each. To handle RVVM2, you just need to make the same change as for RVVM1 in the RVVM2 tests defined a few lines later.

Thanks again, and happy to approve #602 when the latter change is made.

@orlitzky
Copy link
Contributor

orlitzky commented Nov 5, 2024

Thanks! Everything is becoming clearer now. I hadn't made the connection between the M2 in RVVM2 and and the "m2" in the function name.

I've updated the PR. The two tests for RVVM1 and RVVM2 are identical now, which is begging for a follow-up PR to consolidate them.

@blapie blapie closed this as completed in 87e73dc Nov 11, 2024
@blapie blapie linked a pull request Nov 11, 2024 that will close this issue
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 a pull request may close this issue.

5 participants