[FFmpeg-devel] [RFC] [PATCH 0/7] RISC-V V vector length dealings

[Lynne]

Sep 28, 2022, 08:03 by remi at remlab.net:

> Le 28 septembre 2022 00:32:42 GMT+03:00, Lynne <dev at lynne.ee> a écrit :
> >Sep 27, 2022, 22:04 by remi at remlab.net:
>
>>> Hello,
>>>
>>> As a general rule, scalable vector instruction sets should be used with the
>>> largest possible vector length. There are however a number of operations that
>>> just happen with a fixed size, and this patchset exhibits the simplest one I
>>> could find. The proper RISC-V Vector extension guarantees a minimum vector
>>> length of 128 bits. In theory though the underlying specification also allows
>>> for (embedded designs with) only 32 or 64 bits per vector.
>>>
>>> The RFC is how should this be dealt with? The simplest possibibility is to
>>> simply assume 128 bits. Indeed, I am not aware of any actual or proposed
>>> processor IP with shorter-than-128-bit vectors, even less so, one upon which
>>> FFmpeg would be used. For what it is worth, ARM SVE guarantees a minimum of
>>> 128 bits per vector too. In that case, we can drop the first patch, and
>>> simplify the following ones.
>>>
>>> Another option is to expose the vector length via the CPU flags as proposed
>>> earlier by Lynne. Though this is unorthodox the vector length is not a proper
>>> flag. The vector length can readily be retrieved from a read-only unprivileged
>>> CSR, and this patchset instead introduces a simple inline wrapper therefore.
>>> The downside of this approach is that this is nominally undefined behaviour,
>>> and typically will raise a SIGILL, if the processor does not support the
>>> vector extension.
>>>
> >Where's the undefined behavior? If it's guarded by an if, the
> >function will return the maximum length. I don't mind that it's not
> >a cpuflag.
>
>>
>>
> >_______________________________________________
> >ffmpeg-devel mailing list
> >ffmpeg-devel at ffmpeg.org
> >https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>>
>>
> >To unsubscribe, visit link above, or email
> >ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>
>>
>>
>
> The UB occurs if the helper is called on a CPU without vectors. There is no (I think) UB in the patchset. My point is rather that it might be prone to programming errors, not that they would be actual errors already.
>
> Presumably someone could accidentally insert or move a call to ff_rv_get_vlenb() before the proper CPU flags check, and not notice that it would cause UB on some CPUs.
>
> With that said, if there are no objections to the approach in this series, I'm obviously fine with that.
>

I think it's fine as-is. I'm sure we'll get more fate systems other than your U74
to check for that in the future.
Patchset looks good to me. Will apply in a few hours.