RISC-V RVV extension #666
RISC-V RVV extension #666
Conversation
|
Rebased on top of the 1.68 release taking a commit from source forge branch: @dragoș Tiselice Tested via: CMake build: Autotools build: Notes:
The way i would like to proceed with this is to make some tests for the functions not covered by the pngtest and then remove the "draft" prefix. My intention is bringing this support to the state it can be safely merged to upstream. |
There was a problem hiding this comment.
This way of checking for extension support doesn't work. It would e.g. parse the following ISA strings as supporting RVV, while they don't: "rv64gc_xtheadvector", "rv64gcb_xventanacondops"
I would recommend using the hwprobe interface: https://docs.kernel.org/arch/riscv/hwprobe.html
There was a problem hiding this comment.
I addressed the issue making this check a one liner.
riscv/filter_rvv_intrinsics.c
Outdated
| @@ -0,0 +1,275 @@ | |||
| /* filter_neon_intrinsics.c - RISC-V Vector optimized filter functions | |||
riscv/palette_rvv_intrinsics.c
Outdated
| __riscv_vsseg4e8_v_u8m1x4( | ||
| riffled_palette, | ||
| __riscv_vcreate_v_u8m1x4( | ||
| __riscv_vget_v_u8m1x3_u8m1(rgb, 0), |
There was a problem hiding this comment.
I would expect a LMUL=2 vrgather to be faster on most hardware. A widen+merge+compress could also be interesting to look at.
|
@ctruta, @svgeesus Your call of course by I wouldn't touch this with a barge pole. The problem is that the reference to the specification, not here but in @mschlaegl original PR #405 in fact refers to a marketing document which does not contain a spec. I continued searching (dumb fool that I am) and I found this apparently genuine web site: https://riscv.org/specifications/ratified/ Well, it's not there but one or other of the links leads to a page on "atlassian.net" which leads to this page: (NOTE for those who aren't used to doing this, it's a wiki). That, you will note, uses the suffix "+Archive" and leads to a specification on someone's Google Drive someone unidentifiable. The entry on the web site comes from the, "Unprivileged Horizontal Committee". Sounds like me, after I'd finish ROTL I checked them out:
This is the same mess as the MIPS instruction set. Not my problem. |
|
The latest RISC-V spec can be found on the riscv-isa-manual github, here is fhe file with the ratified RVV spec: https://github.com/riscv/riscv-isa-manual/blob/main/src/v-st-ext.adoc |
|
When it comes to official spec: Hints in regard to implementation: Useful intrinsic viewer: |
filipwasil
force-pushed
the
riscv-vector-updated-16
branch
2 times, most recently
from
566b408 to
5438353
Compare
|
Thank You for all the comments As I see it now, the implementation I assumed I will rely on looks like a work in progress. Tests do not pass. At this point I propose such a roadmap:
Does this sound like a plan ? |
|
TLDR:
|
filipwasil
force-pushed
the
riscv-vector-updated-16
branch
from
5438353 to
638dfdb
Compare
contrib/riscv-vector/README
Outdated
| @@ -0,0 +1,85 @@ | |||
| OPERATING SYSTEM SPECIFIC ARM NEON DETECTION | |||
filipwasil
force-pushed
the
riscv-vector-updated-16
branch
from
103547c to
21606ec
Compare
|
EDIT: |
filipwasil
force-pushed
the
riscv-vector-updated-16
branch
from
21606ec to
5c7cd57
Compare
filipwasil
force-pushed
the
riscv-vector-updated-16
branch
from
5c7cd57 to
f732c87
Compare
|
Before I start to rewrite the ASM part into c intrinsics, please let me know: are there any concerns which I should address and specifically - is there a need to rewrite it at all ? When it comes to the spec: The specification rev 1.0 is already ratified. Are there any specific documents we need to proceed with these changes ? We have recently pushed docker based CI configuration to https://gitlab.freedesktop.org/pixman/pixman to be able to automate SIMD vectorization testing for RISC-V RVV. Is this a factor for this particular PR ? Having this specific code tested within CI ? As for these changes: Build cmake Build autotools tests: |
|
I defer to @ctruta on this one, I have nothing to contribute here |
|
Thank you for your extraordinary patience. I'm back again at this project, giving your PR a look, and a spin. |
filipwasil
force-pushed
the
riscv-vector-updated-16
branch
from
6449a96 to
638dfdb
Compare
|
I updated the branch because of few reasons:
At this point this commit contains only changes proven to pass all tests and to provide the whole build system related boilerplate code, only supporting the RVV 1.0+ spec as it is and will be the mature revision having more and more extensions and proven By all tests I mean: Have this merged to upstream probably would be a good base for any further optimizations for RISC-V Vector extension which I have to work on, if the help is appreciated. In pixman CI setup we are able to test against future revisions of RISC-V hardware supporting 512b and even 1024b VLEN already, as the existing hardware (like BPi F3) have only 256b. |
|
I pushed one last change for the upcoming v1.6.48 which I want to release tomorrow. It's for helping me with the testing of CMake-based cross-platform builds, because I had quite a few headaches as I was working on putting these onto the CI bots. (For context: we never tested cross-platform builds on the CI bots before, but I want to change that.) Just so you know: in my local workflow, I'm running Ubuntu under QEMU, and I want to set up FreeBSD as well. |
About that: any tips, links, do's, dont's that you happen to know and you think I might want to know -- I will greatly appreciate. |
|
@ctruta : the lines of the form:
have not been permitted before. This is because the maintainer regards (regarded?) things that failed with -Wundef as inadmissible; this is a large cause of massive # directive nesting (and a major annoyance to me.) While it is stated that the changes were copied from the ARM ones the ARM tests have the required checks that the macros being tested are defined. It would vastly, vastly simplify the #if mess were the ANSI-C rules (not-defined := 0) followed but I suggest that this might be a 1.8 thing. It's an easy fix @filipwasil PITA but easy. |
@jbowler I hereby give it to you in writing that, if you want to blame it on my inability to tell the difference between an unintentionally-uninitialized macro which is obviously a bug -- versus -- an intentionally-uninitialized macro which is obviously a feature, I promise not to take it personally 🤪 The people who understand my statement "I wouldn't touch Perl code without
Speaking about the aforementioned PITA: let me please not get myself started on As a general observation: every single macro in libpng is there for a reason. And yet, all of those macros, combined, are an over-engineered mess which accreted over the decades, black-hole-style. If we need to keep doing anything in v1.8 is to keep cleaning as much of that as possible. I firmly believe that any soul who somehow ended up entrapped in a room with the libpng code (that they must read, and not only that, but also comprehend) will thank us later. It can be done. (Trust Me™️) |
|
Building
Few notes:
|
|
I will improve the detection mechanism for autotools and remove the "draft" prefix when its all tested |
|
FYI, I acknowledge your very long wait and I appreciate your very long patience. I will apply the finishing touches to the v1.6.48 release, then I will then I will initiate v1.6.49, then I will apply your submission, in this particular order. |
|
Aaand..... we're in! 🚀 ✨ I must admit that I have yet to test this in my own RVV VM. Last time I did run tests, it was when I put in the files ci/targets/linux/ci_env.riscv64-linux-gnu.sh and ci/targets/freebsd/ci_env.riscv64-unknown-freebsd.sh -- so, they're in there, and fortunately, it's far easier to run cross-platform tests nowadays. Let's let this brew for a couple of weeks, and I'll run my own tests properly in the meantime. |
|
@filipwasil could you please fix (or rewrite) the paragraphs in this comment: Line 149 in ffb8e8b I integrated them no problem, but they're still bad copy-pasta. I don't suppose you guys actually intended to write /*[...] the compiler will define I think if you just force-push away the commits that I just integrated (with tiny stylistic changes differences that appear to irreconcilable to GitHub's rebase bot, as you can see), and then you add in a new commit which contains with those fixes -- that should work just fine in this workflow. |
|
I appreciate all the comments. I will continue working on this next week -
adjusting the comments and making some extra tests. Thank You.
czw., 1 maj 2025, 18:22 użytkownik Cosmin Truta ***@***.***>
napisał:
… *ctruta* left a comment (pnggroup/libpng#666)
<#666 (comment)>
@filipwasil <https://github.com/filipwasil> could you please fix (or
rewrite) the paragraphs in this comment:
https://github.com/pnggroup/libpng/blob/ffb8e8b26f2c18798863a173945cf4f292ab1f0a/pngpriv.h#L149
I integrated them no problem, but they're still bad copy-pasta. I don't
suppose you guys actually intended to write */*[...] the compiler will
define __RVV__ and we can rely unconditionally on NEON instructions not
crashing [...]*/*
I think if you just force-push away the commits that I just integrated
(with tiny stylistic changes differences that appear to irreconcilable to
GitHub's rebase bot, as you can see), and then you add in a new commit
which contains with those fixes -- that should work just fine in this
workflow.
—
Reply to this email directly, view it on GitHub
<#666 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOENPMHH6QU4FVDTVQEFMT24JC3ZAVCNFSM6AAAAABYQ2Q6TWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNBVGE4DAMBTHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
filipwasil
force-pushed
the
riscv-vector-updated-16
branch
3 times, most recently
from
394bb02 to
546fe8e
Compare
|
After tests i updated comments and some definitions as well as this table: Now all should be more complete and according to my tests : correct for autotools and for CMAKE -> on x86 we can explicitly try to compile the risc-v or ARM but then the compilation fails because of missing headers. I assumed that this is considered correct. |
No; there is a longstanding bug with cmake builds of ARM but they fail at the link step; no missing headers. Both RISC-V and ARM (i.e. 32-bit) currently build just fine with configure on AMD64 (i.e. x86) with the current libpng16:HEAD In the RISCV build the RISCV objects (riscv/*.o) do not define any of the expected symbols (this is with -march=rv64g_v) and object modules are being build for The fact that RISCV builds do not build anything however is perhaps unintended. |
|
Thanks for the explanation @jbowler . Just one note in regard to:
-march=rv64gv The underscore is used to separate multi-letter or non-standard extensions, but in the case of the vector extension, the standard GCC and RISC-V convention is to append v directly after the other extensions, without an underscore2. |
|
I did an update to support the runtime check build for both build systems and updated the table once more |
|
Please don't mind my use of GitHub through the command line, which sometimes confuses the web interface. I'll close this PR as "not merged", even though all submissions sent to us so far are already in. I'll let this brew for a few more days, and then I'll publish libpng-1.6.49 with RISC-V support. Thank you very much @filipwasil @dragostis @mschlaegl for your great work on this submission, and thank you everybody who contributed comments and reviews. |
|
There's maybe a few typos to sort out before releasing: Line 15608 in 7108843 libpng/contrib/riscv-rvv/README Line 1 in 7108843 Line 1 in 7108843 Lines 62 to 65 in 7108843 |
A-ha! |
|
I wrote:
So, yeah, I did spot the copy-pasta two weeks ago, but not earlier today, apparently. Thanks for the catch, @kmilos. If the tests are passing, that's great. But then, there's this: Fix wanted, please :-) |
filipwasil
force-pushed
the
riscv-vector-updated-16
branch
from
94cd171 to
9ff6e6f
Compare
|
Thanks for Your time reviewing the PR. I went through the comments. found two more things. fixed. |
|
Ok, so, we're almost finished, but still not entirely finished. @filipwasil if run locally the command you will see a bunch of changes. Everything except for "ltmain.sh" will need to be updated, eventually. I will run this command, so you don't have to, but... could you please run it as well, at least locally, to make sure that things don't break? The autogen command (which runs autoreconf underneath) will apply changes not only to script code but also to script comments, so please review those ones also. In case you might be unfamiliar with the autotools, here's how to get them, e.g., with Homebrew: No, seriously, we're getting there... we're getting there... :-) |
7 participants
Updating and testing autotools build and CMake build
Renaming riscv-vector to riscv-rvv
Changes implementation of few functions because they do not compile for RVV 1.0 via gcc 14.0.1 20240412
Corrected and added few flag names and definitions
Solved merge conflicts