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

Merge risc-v branch to develop #4472

Merged

Conversation

sergei-lewis
Copy link
Contributor

No description provided.

xianyi and others added 30 commits August 24, 2022 19:24
`make NOFORTRAN=1 CC=gcc`
…non-isa/rvv-intrinsic-doc (in particular, __riscv_ prefixes for rvv intrinsics)

* fix multiple numerical stability and corner case issues
* add a script to generate arbitrary gemm kernel shapes
* add a generic zvl256b target to demonstrate large gemm kernel unrolls
…vv-intrinsics

update riscv intrinsics for latest spec
add riscv level3 C,Z kernel functions.
Add prefix (_riscv) for all riscv intrinsics
Update some intrinsics' parameter, like vfredxxxx, vmerge
RISC-V for new intrinsic API changes
fix wrong vr = VFMVVF_FLOAT(0, vl);
fix wrong vr = VFMVVF_FLOAT(0, vl); in symv_L_rvv.c and symv_U_rvv.c
Add rvv support for zsymv and active rvv support for zhemv
Changes masked intrinsics from _m to _mu and reintroduces maskedoff argument.
During the last iteration of some RVV operations, accumulators can get overwritten when VL < VLMAX and tail policy is agnostic.
Commit changes intrinsics tail policy to undistrubed.
During the last iteration of some RVV operations, accumulators can get overwritten when VL < VLMAX and tail policy is agnostic.
Commit changes intrinsics tail policy to undistrubed.
Current RVV x280 target depends on vlen=512-bits for Level 3 operations.
Commit adds generic target that supports vlen=128-bits.
New target uses the same scalable kernels as x280 for Level 1&2 operations, and autogenerated kernels for Level 3 operations.
Functional correctness of Level 3 operations tested on vlen=128-bits using QEMU v8.1.1 for ctests and BLAS-Tester.
@sergei-lewis sergei-lewis marked this pull request as draft February 1, 2024 10:54
@martin-frbg
Copy link
Collaborator

hey, I know people are getting impatient but my plan was to merge #4457 first (which got opened against the branch) but also make sure that its new tests do not blow up all the other platforms... would you prefer to defer that ?

@sergei-lewis
Copy link
Contributor Author

sergei-lewis commented Feb 1, 2024

Makes perfect sense, completely happy to wait with this PR until everything is in place and update this PR as necessary. I've marked the PR as draft, will unmark once everyone is happy with what is happening (or close it if we decide to go for a different strategy).

I've tested this on x86, RISC64_GENERIC, RISCV64_ZVL128B and RISCV64_ZVL256B locally. A big part of my reason for kicking a PR is, given the size of the changeset and the fact that it includes many changes outside riscv64-specific modules I am super keen to kick off discussion early and also take advantage of OpenBLAS CI to exercise all the other configurations to get visibility on any potential problems.

@sergei-lewis sergei-lewis force-pushed the dev/slewis/merge-from-riscv branch from a3b0ef6 to 452741b Compare February 1, 2024 11:14
@martin-frbg
Copy link
Collaborator

Must admit that the size and range of this changeset is worrying me a bit as well, it used to look a lot more harmless when it was just a trivial update of kernel/riscv64... maybe you're right and rebasing the new utests after the merge makes more sense...

@sergei-lewis sergei-lewis reopened this Feb 1, 2024
@sergei-lewis
Copy link
Contributor Author

An alternative strategy might be to rebase risc-v on current develop, deal with the fallout on the branch, then kick a new PR based on that. It might lead to a smaller changeset, and/or make it feasible to split the merge into several smaller PRs rather than one giant one; but it's no magic bullet, and it's not immediately clear that things would be better that way instead of worse.

@martin-frbg
Copy link
Collaborator

Yes, lets go with this one unless anyone complains in the next few hours. The utest extensions are going to open their own can of worms that is totally unrelated to risc-v, and merging develop into risc-v would appear to create at least as many merge conflicts and chances for fallout than doing it here. Add to that that we do not have any CI job that runs on actual RISC-V hardware yet, so the chances of detecting fallout might be smaller.
It is just that I was fixated on getting other PRs merged before tackling this one.

@martin-frbg
Copy link
Collaborator

Looking good so far, only two timeouts due to slow CI hardware

@martin-frbg martin-frbg marked this pull request as ready for review February 3, 2024 09:52
@martin-frbg martin-frbg self-requested a review February 3, 2024 09:53
Copy link
Collaborator

@martin-frbg martin-frbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@martin-frbg martin-frbg added this to the 0.3.27 milestone Feb 3, 2024
@sl236
Copy link

sl236 commented Feb 3, 2024

I’m happy if you’re happy!

@martin-frbg martin-frbg merged commit 27816fa into OpenMathLib:develop Feb 3, 2024
61 of 64 checks passed
@martin-frbg
Copy link
Collaborator

Hmm... now that I have merged this, the hang in the C910V qemu job appears to be persistent :( @sergei-lewis can you check if you see it on actual hardware too ?

@sergei-lewis
Copy link
Contributor Author

Apologies, was travelling. Will take a look ASAP.

@martin-frbg
Copy link
Collaborator

I've switched AXPY back to generic kernels to get around the hang, but I suspect this may be just another thing that only happens in qemu.

@sergei-lewis
Copy link
Contributor Author

PR 4497 fixes the hang and reenables the vectorised AXPY kernels on C910V.

Any reason we shouldn't add CI testing for the new ZVL128B / ZVL256B targets, btw? I'll put together a PR if not.

@martin-frbg
Copy link
Collaborator

thanks. no reason to not include them in CI, it isn't that long since the branch got merged, is it ? :)

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.

9 participants