[FFmpeg-devel] [PATCH 05/31] lavu/cpu: CPU flags for the RISC-V Vector extension

[Rémi Denis-Courmont]

Le 26 septembre 2022 09:51:43 GMT+03:00, Lynne <dev at lynne.ee> a écrit :
>Sep 25, 2022, 16:25 by remi at remlab.net:
>
>> From: Rémi Denis-Courmont <remi at remlab.net>
>> -    if ((flags & AV_CPU_FLAG_RVD) && !(flags & AV_CPU_FLAG_RVF)) {
>> +    if ((flags & AV_CPU_FLAG_RV_ZVE64D) && !(flags & AV_CPU_FLAG_RV_ZVE64X)) {
>> +        av_log(NULL, AV_LOG_WARNING, "RV%s implied by specified flags\n",
>> +               "_ZVE64X");
>> +        flags |= AV_CPU_FLAG_RV_ZVE64X;
>> +    }
>> +
>> +    if ((flags & AV_CPU_FLAG_RV_ZVE64D) && !(flags & AV_CPU_FLAG_RV_ZVE32F)) {
>> +        av_log(NULL, AV_LOG_WARNING, "RV%s implied by specified flags\n",
>> +               "_ZVE32F");
>>
>
>I remember someone complaining about NULL contexts in av_log (mkver?).
>I think it's okay, but I have no opinion on this.

I don't particularly like them either but there is nowhere to get the log context from, in this case. To fix this, I guess we would need to break the API and the ABI.

This is the same as the existing x86 code anyhow. Any solution should be common to both platforms.

>
>> +        flags |= AV_CPU_FLAG_RV_ZVE32F;
>> +    }
>> +
>> +    if ((flags & (AV_CPU_FLAG_RV_ZVE64X | AV_CPU_FLAG_RV_ZVE32F))
>> +        && !(flags & AV_CPU_FLAG_RV_ZVE32X)) {
>> +        av_log(NULL, AV_LOG_WARNING, "RV%s implied by specified flags\n",
>> +               "_ZVE32X");
>> +        flags |= AV_CPU_FLAG_RV_ZVE32X;
>> +    }
>> +
>> +    if ((flags & AV_CPU_FLAG_RV_ZVE64D) && !(flags & AV_CPU_FLAG_RVD)) {
>> +        av_log(NULL, AV_LOG_WARNING, "RV%s implied by specified flags\n", "D");
>> +        flags |= AV_CPU_FLAG_RVD;
>> +    }
>> +
>> +    if ((flags & (AV_CPU_FLAG_RVD | AV_CPU_FLAG_RV_ZVE32F))
>> +        && !(flags & AV_CPU_FLAG_RVF)) {
>>  av_log(NULL, AV_LOG_WARNING, "RV%s implied by specified flags\n", "F");
>>  flags |= AV_CPU_FLAG_RVF;
>>  }
>> @@ -50,6 +75,11 @@ int ff_get_cpu_flags_riscv(void)
>>  ret |= AV_CPU_FLAG_RVF;
>>  if (hwcap & HWCAP_RV('D'))
>>  ret |= AV_CPU_FLAG_RVD;
>> +
>> +    /* The V extension implies all Zve* functional subsets */
>> +    if (hwcap & HWCAP_RV('V'))
>> +        ret |= AV_CPU_FLAG_RV_ZVE32X | AV_CPU_FLAG_RV_ZVE64X
>> +             | AV_CPU_FLAG_RV_ZVE32F | AV_CPU_FLAG_RV_ZVE64D;
>>  #endif
>>  
>>  #ifdef __riscv_i
>> @@ -60,6 +90,20 @@ int ff_get_cpu_flags_riscv(void)
>>  #if (__riscv_flen >= 64)
>>  ret |= AV_CPU_FLAG_RVD;
>>  #endif
>> +#endif
>> +
>> +    /* If RV-V is enabled statically at compile-time, check the details. */
>> +#ifdef __riscv_vectors
>> +    ret |= AV_CPU_FLAG_RV_ZVE32X;
>> +#if __riscv_v_elen >= 64
>> +    ret |= AV_CPU_FLAG_RV_ZVE64X;
>> +#endif
>> +#if __riscv_v_elen_fp >= 32
>> +    ret |= AV_CPU_FLAG_RV_ZVE32F;
>> +#if __riscv_v_elen_fp >= 64
>> +    ret |= AV_CPU_FLAG_RV_ZVE64F;
>> +#endif
>> +#endif
>>  #endif
>>  
>>  return ret;
>> diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c
>> index e1135a84ac..f7d108e8ea 100644
>> --- a/tests/checkasm/checkasm.c
>> +++ b/tests/checkasm/checkasm.c
>> @@ -233,9 +233,13 @@ static const struct {
>>  { "VSX",      "vsx",      AV_CPU_FLAG_VSX },
>>  { "POWER8",   "power8",   AV_CPU_FLAG_POWER8 },
>>  #elif ARCH_RISCV
>> -    { "RVI",      "rvi",      AV_CPU_FLAG_RVI },
>> -    { "RVF",      "rvf",      AV_CPU_FLAG_RVF },
>> -    { "RVD",      "rvd",      AV_CPU_FLAG_RVD },
>> +    { "RVI",        "rvi",       AV_CPU_FLAG_RVI },
>> +    { "RVF",        "rvf",       AV_CPU_FLAG_RVF },
>> +    { "RVD",        "rvd",       AV_CPU_FLAG_RVD },
>> +    { "RV_Zve32x",  "rv_zve32x", AV_CPU_FLAG_RV_ZVE32X },
>> +    { "RV_Zve32f",  "rv_zve32f", AV_CPU_FLAG_RV_ZVE32F },
>> +    { "RV_Zve64x",  "rv_zve64x", AV_CPU_FLAG_RV_ZVE64X },
>> +    { "RV_Zve64d",  "rv_zve64d", AV_CPU_FLAG_RV_ZVE64D }, 
>>
>
>I get that this is the official name for the extension, but... what about
>simplifying it to something less like a password, like RVV32I/RVV32F/RVV64I/RVV64F?

There are 2 prefixes: Zve for vector element, and Zvl for vector bit length. If we drop the E of element, it gets confusing.

Maybe we could use RVV_{I,F}{32,64} if you want to drop the gratuitous Z... ?
Inline...