| Message ID | 20230703024412.3229854-1-raj.khem@gmail.com |
|---|---|
| State | Accepted |
| Commit | a7b3c0203fc059db13595e0d0935e50979d2f41c |
| Headers | show |
| Series | [FFmpeg-devel] libswscale/riscv: Fix syntax of vsetvli | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Hi, The diff is ok. I even have a slight incline in favour thereof for the sake of consistency. Yet, I may be vainly pedantic but I do have problems with the description. Le 3 juillet 2023 05:44:12 GMT+03:00, Khem Raj <raj.khem@gmail.com> a écrit : >Add missing operand The spec does explicitly make the mask and tail policy mandatory, but I can't see any such requirements for the group multiplier. To the contrary, there are several examples in the spec _without_ explicit multiplier. In reality, `vsetvli` (and `vsetivli`) merely transfers an immediate value to the vector configuration register. Missing fields are to be left at zero for forward compatibility, and `m1` happens to encode as 0b000. > Which clang complains about but gcc assumes it to be >'m1' if not specifiied. The technical term for the RISC-V support in LLVM AS version 15 and earlier is "useless junk". If you want to compile FFmpeg on RISC-V with Clang, you *must* disable the integrated AS and use binutils GNU/as instead. The Linux RISC-V kernel altogether gave up on LLVM entirely, requiring GCC for RVV, so they're even stricter (and without kernel support, FFmpeg support is obviously useless). *Hopefully* LLVM gets their act together by release 16, and ship a usable assembler, rather than tell us to use automatic RVV vectorisation (which *is* a release 16 feature, though it was half-baked last time I tried). > >Fixes building with clang More like bug-compatible work-around than fix, AFAIU. >| src/libswscale/riscv/rgb2rgb_rvv.S:88:25: error: operand must be e[8|16|32|64|128|256|512|1024],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu] Do you have a reference to the Github RVV spec to validate this, that I overlooked, or it's just misled and misleading spew from LLVM? >| vsetvli t4, t3, e8, ta, ma >| ^ > >Signed-off-by: Khem Raj <raj.khem@gmail.com> >--- > libswscale/riscv/rgb2rgb_rvv.S | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/libswscale/riscv/rgb2rgb_rvv.S b/libswscale/riscv/rgb2rgb_rvv.S >index 5626d906eb..bbdfdbebbc 100644 >--- a/libswscale/riscv/rgb2rgb_rvv.S >+++ b/libswscale/riscv/rgb2rgb_rvv.S >@@ -85,7 +85,7 @@ func ff_interleave_bytes_rvv, zve32x > mv t3, a3 > addi a4, a4, -1 > 2: >- vsetvli t4, t3, e8, ta, ma >+ vsetvli t4, t3, e8, m1, ta, ma > sub t3, t3, t4 > vle8.v v8, (t0) > add t0, t4, t0
On Mon, Jul 3, 2023 at 1:00 AM Rémi Denis-Courmont <remi@remlab.net> wrote: > > Hi, > > The diff is ok. I even have a slight incline in favour thereof for the sake of consistency. Yet, I may be vainly pedantic but I do have problems with the description. > > Le 3 juillet 2023 05:44:12 GMT+03:00, Khem Raj <raj.khem@gmail.com> a écrit : > >Add missing operand > > The spec does explicitly make the mask and tail policy mandatory, but I can't see any such requirements for the group multiplier. To the contrary, there are several examples in the spec _without_ explicit multiplier. > > In reality, `vsetvli` (and `vsetivli`) merely transfers an immediate value to the vector configuration register. Missing fields are to be left at zero for forward compatibility, and `m1` happens to encode as 0b000. > I just replicated what GAS is doing here implicitly. > > Which clang complains about but gcc assumes it to be > >'m1' if not specifiied. > > The technical term for the RISC-V support in LLVM AS version 15 and earlier is "useless junk". If you want to compile FFmpeg on RISC-V with Clang, you *must* disable the integrated AS and use binutils GNU/as instead. > > The Linux RISC-V kernel altogether gave up on LLVM entirely, requiring GCC for RVV, so they're even stricter (and without kernel support, FFmpeg support is obviously useless). > > *Hopefully* LLVM gets their act together by release 16, and ship a usable assembler, rather than tell us to use automatic RVV vectorisation (which *is* a release 16 feature, though it was half-baked last time I tried). > I am on clang/llvm trunk ( future 17.0 release ), I will also open an issue with llvm on github regarding this. > > > >Fixes building with clang > > More like bug-compatible work-around than fix, AFAIU. I can do it although I did not find a relevant section in spec about these being optional, I did see examples omitting them though. > > >| src/libswscale/riscv/rgb2rgb_rvv.S:88:25: error: operand must be e[8|16|32|64|128|256|512|1024],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu] > > Do you have a reference to the Github RVV spec to validate this, that I overlooked, or it's just misled and misleading spew from LLVM? Perhaps the latter but I also did not see reference to the former. Here is a small example a.S #.option arch, +zve32x # OK #vsetvli t4, t3, e8, m1, ta, ma # BAD vsetvli t4, t3, e8, ta, ma clang -target riscv64 -march=rv64izve32x1p0 a.S -c > > >| vsetvli t4, t3, e8, ta, ma > >| ^ > > > >Signed-off-by: Khem Raj <raj.khem@gmail.com> > >--- > > libswscale/riscv/rgb2rgb_rvv.S | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > >diff --git a/libswscale/riscv/rgb2rgb_rvv.S b/libswscale/riscv/rgb2rgb_rvv.S > >index 5626d906eb..bbdfdbebbc 100644 > >--- a/libswscale/riscv/rgb2rgb_rvv.S > >+++ b/libswscale/riscv/rgb2rgb_rvv.S > >@@ -85,7 +85,7 @@ func ff_interleave_bytes_rvv, zve32x > > mv t3, a3 > > addi a4, a4, -1 > > 2: > >- vsetvli t4, t3, e8, ta, ma > >+ vsetvli t4, t3, e8, m1, ta, ma > > sub t3, t3, t4 > > vle8.v v8, (t0) > > add t0, t4, t0
Le maanantaina 3. heinäkuuta 2023, 18.52.13 EEST Khem Raj a écrit : > > *Hopefully* LLVM gets their act together by release 16, and ship a usable > > assembler, rather than tell us to use automatic RVV vectorisation (which > > *is* a release 16 feature, though it was half-baked last time I tried). > I am on clang/llvm trunk ( future 17.0 release ), I will also open an > issue with llvm on github regarding this. If I were you, I would first open an issue on the riscv-v-spec Github to seek clarification (though I'm not sure if it's open for bug submission?). > > >Fixes building with clang > > > > More like bug-compatible work-around than fix, AFAIU. > > I can do it although I did not find a relevant section in spec about > these being optional, I did see examples omitting them though. > > > >| src/libswscale/riscv/rgb2rgb_rvv.S:88:25: error: operand must be > > >| e[8|16|32|64|128|256|512|1024],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]> > > Do you have a reference to the Github RVV spec to validate this, that I > > overlooked, or it's just misled and misleading spew from LLVM? > Perhaps the latter but I also did not see reference to the former. > Here is a small example > > a.S > #.option arch, +zve32x > # OK > #vsetvli t4, t3, e8, m1, ta, ma > # BAD > vsetvli t4, t3, e8, ta, ma > > clang -target riscv64 -march=rv64izve32x1p0 a.S -c Well, yes but the idea is to keep V disabled in the target flags, and do run- time detection... Especially so far, while RVV hardware remains unobtainium, the compiler can't assume that V is supported, or else... And so LLVM AS has been so far unusable for both FFmpeg and kernel alike.
diff --git a/libswscale/riscv/rgb2rgb_rvv.S b/libswscale/riscv/rgb2rgb_rvv.S index 5626d906eb..bbdfdbebbc 100644 --- a/libswscale/riscv/rgb2rgb_rvv.S +++ b/libswscale/riscv/rgb2rgb_rvv.S @@ -85,7 +85,7 @@ func ff_interleave_bytes_rvv, zve32x mv t3, a3 addi a4, a4, -1 2: - vsetvli t4, t3, e8, ta, ma + vsetvli t4, t3, e8, m1, ta, ma sub t3, t3, t4 vle8.v v8, (t0) add t0, t4, t0
Add missing operand which clang complains about but gcc assumes it to be 'm1' if not specifiied. Fixes building with clang | src/libswscale/riscv/rgb2rgb_rvv.S:88:25: error: operand must be e[8|16|32|64|128|256|512|1024],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu] | vsetvli t4, t3, e8, ta, ma | ^ Signed-off-by: Khem Raj <raj.khem@gmail.com> --- libswscale/riscv/rgb2rgb_rvv.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)