[FFmpeg-devel] [PATCH 02/15] avfilter/vf_bwdif: Add common macros and consts for aarch64 neon

[John Cox]

On Mon, 3 Jul 2023 00:02:27 +0300 (EEST), you wrote:

>On Sun, 2 Jul 2023, Martin Storsjö wrote:
>
>> On Sun, 2 Jul 2023, John Cox wrote:
>>
>>> On Sun, 2 Jul 2023 00:35:14 +0300 (EEST), you wrote:
>>> 
>>>> On Thu, 29 Jun 2023, John Cox wrote:
>>>> 
>>>>> Add macros for dual scalar half->single multiply and accumulate
>>>>> Add macro for shift, saturate and shorten single to byte
>>>>> Add filter constants
>>>>> 
>>>>> Signed-off-by: John Cox <jc at kynesim.co.uk>
>>>>> ---
>>>>> libavfilter/aarch64/vf_bwdif_neon.S | 46 +++++++++++++++++++++++++++++
>>>>> 1 file changed, 46 insertions(+)
>>>>> 
>>>>> +
>>>>> +        .align 16
>>>> 
>>>> Note that .align for arm is power of two; this triggers a 2^16 byte
>>>> alignment here, which certainly isn't intended.
>>> 
>>> Yikes! I'll swap that for a .balign now I've looked that up
>>> 
>>>> But in general, the usual way of defining constants is with a
>>>> const/endconst block, which places them in the right rdata section instead
>>>> of in the text section. But that then requires you to use a movrel macro
>>>> for accessing the data, instead of a plain ldr instruction.
>>> 
>>> Yeah - arm has a history of mixing text & const - I went with the
>>> simpler code. Is this a deal breaker or can I leave it as is?
>>
>> I wouldn't treat it as a deal breaker as long as it works across all 
>> platforms - even if consistency with the code style elsewhere is preferred, 
>> but IIRC there may be issues with MS armasm (after passed through 
>> gas-preprocessor). IIRC there might be issues with starting out with straight 
>> up content without the full setup made by the function/const macros. OTOH I 
>> might have fixed that in gas-preprocessor too...
>>
>> Last time around, the patchset failed building in that configuration due ot 
>> the out of range alignment, I'll see how it fares now.
>
>I'm sorry, but I'd just recommend you to go with the const macros.
>
>Your current patch fails because gas-preprocessor, 
>https://github.com/ffmpeg/gas-preprocessor, doesn't support the .balign 
>directive, it only recognizes .align and .p2align. (Extending it to 
>support it would be trivial though.)
>
>If I change your code to ".align 4", I get the following warning:
>
>\home\martin\code\ffmpeg-msvc-arm64\libavfilter\aarch64\vf_bwdif_neon.o.asm(1011) 
>: warning A4228: Alignment value exceeds AREA alignment; alignment not 
>guaranteed
>         ALIGN 16
>
>Since we haven't started any section, apparently armasm defaults to a 
>section with 4 byte alignment.
>
>But anyway, regardless of the alignment, it later fails with this error:
>
>\home\martin\code\ffmpeg-msvc-arm64\libavfilter\aarch64\vf_bwdif_neon.o.asm(1051) 
>: error A2504: operand 2: Expected address
>         ldr             q0, coeffs
>
>
>So I would request you to just go with the macros we use elsewhere. The 
>gas-preprocessor/armasm setup doesn't support/expect any random assembly, 
>but the disciplined subset we normally write. In most cases, we 
>essentially never write bare directives in the code, but only use the 
>macros from asm.S, which are set up to handle portability across all 
>supported platforms and their toolchains.

OK - will do.

JC

>// Martin