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 <j...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
