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

["Martin Storsjö" <martin@martin.st>]

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@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.

// Martin
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".