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

Add vector instructions to RISC-V emitter #16829

Merged
merged 11 commits into from
Jan 22, 2023

Conversation

unknownbrackets
Copy link
Collaborator

@unknownbrackets unknownbrackets commented Jan 22, 2023

Wanted to read through the available instructions in the spec which has been final for a bit, so also added encodings. Based on Vector Extension spec version 1.0.

The assembler syntax is i.e. vadd.vv v0, v1, v2 or vadd.vx v0, v1, f2 so decided to keep that pattern... it's probably clearer. Also some variants don't exist, i.e. vmfge.vf exists but vmfge.vv doesn't.

Should get around to implementing some RISC-V jitting, though I keep wanting to start with the vertex decoder and being disappointed by how slow my test device is (due to Vulkan issues), sigh...

-[Unknown]

@hrydgard
Copy link
Owner

This is the spec, right? https://github.com/riscv/riscv-v-spec/releases/tag/v1.0

Could add a link in the header file and the PR description just for documentation's sake.

Yeah I haven't heard back more yet about the Vulkan issues unfortunately...

@hrydgard hrydgard merged commit 4dcd2a1 into hrydgard:master Jan 22, 2023
@unknownbrackets unknownbrackets deleted the riscv-vec branch January 22, 2023 14:55
@unknownbrackets
Copy link
Collaborator Author

It was ratified so I guess it's probably "2.0" now:
https://wiki.riscv.org/display/HOME/Recently+Ratified+Extensions

I'd prefer to link to the "integrated" spec but for some reason it hasn't been updated since 2019 (vector was ratified in 2021-11.) Even the GitHub builds of the integrated one have the 0.7 version of vector, which confused me before (and is why I didn't add them to the emitter earlier.)

I guess I'm not the only person who thinks it's gotten unclear: riscv/riscv-isa-manual#904

-[Unknown]

@unknownbrackets unknownbrackets added this to the v1.15.0 milestone Jan 22, 2023
@gamelaster
Copy link

(Sorry, I don't have much insight into the PPSSPP codebase, so I might ask trivial questions.)
Is the vector extension optional? If yes, do we know how much it decreases performance when disabled?
Because, there is one problem with vector extension. The RISC-V cores (T-Head C906 or C910) used in latest cheap Chinese SoCs (Allwinner D1, JH7110, TH1520) supports only Vector extension version 0.7.1 , which is not compatible with 1.0 specification.
Eventually, would it be possible to add support for both vector extension versions?
Thanks!

@hrydgard
Copy link
Owner

hrydgard commented May 3, 2023

@gamelaster it's early days for RISC-V support in PPSSPP, and not a lot of vector instructions are used yet AFAIK. This PR just adds them to the instruction emitter.

I'm sure it would be possible to add both, but it seems really weird and unfortunate that devices are coming out with two different incompatible versions of an instruction set, that's just a royal pain to support. Guess that's the risk (ha) with open source ISAs...

@unknownbrackets
Copy link
Collaborator Author

Currently, it never uses any vector instructions - so there's no performance benefit here currently. While it's possible a common subset of the two might be supported in the future, I would recommend you assume that only ratified versions of V will be supported.

At the time these chips were made, 0.7.1 was all that existed. So they took a bet that it would be largely compatible with the final ratified version and figured this was better than not supporting vector at all. I'm not convinced these chips will end up in popular RISC-V devices if RISC-V takes off. A more likely story is that ARM's licensing changes motivate some people to consider RISC-V and switch chips a couple years down the line at the earliest, but with newer chips that support the latest spec. If the 0.7.1-only chips are a substantial part of the RISC-V market in 2 years, that probably indicates things have gone badly for RISC-V (which is also a perfectly possible story.)

-[Unknown]

@gamelaster
Copy link

gamelaster commented May 4, 2023

@unknownbrackets the issue is, that T-Head had opportunity and chance to upgrade their existing C906 and C910 cores to have Vector 1.0 extension, and even after pressure from the community, the latest SoCs as JH7110 and TH1520 still have 0.7.1, even they were released only few months ago.

Of course in future the RISC-V SoCs (hopefully) use only latest versions of extensions, although, even now there are interesting boards such as Star64, VisionFive2 and Sipeed LM4A, whose have SoCs with quite decent CPU and GPU power to be able to run PPSSPP at some point I guess (+ from latest tests, it seems that AMD GPUs will be able to run on JH7110 based SoCs), so after the mainline Linux and GPU drivers will get into better shape, I think there will be some community interest to bring up PPSSPP on those boards very soon.
image

(Note, the "Light Blue" Benchmark results are exactly the compiler, which support Vector extension 0.7.1 and special T-Head vendor instructions. This reminds me, that those T-Head instructions can be implemented too, which probably can introduce various performance improvements?)

@unknownbrackets
Copy link
Collaborator Author

It's not the release date that matters. Vector 1.0 was only ratified 2021-11 from a draft in 2021--06. Because of the time it takes to produce chips, that means you wouldn't expect to say any chips supporting Vector 1.0 until 2023-06 at the very earliest. The draft for 0.7.1 was released in 2019-06, so it's reasonable that chips that came out in 2021 would use that (I think that's when the U74 came out.)

Anyway, it'll still be possible to compile PPSSPP with whatever clang/gcc support, including vector 0.7.1 or T-Head extensions. What PPSSPP's emitters support would affect what its jit uses, and that's not even built out yet - even having a jit without vector support at all will represent a decent perf improvement. It will take time.

-[Unknown]

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.

3 participants