From: "Martin Storsjö" <martin@martin.st>
To: "Swinney, Jonathan" <jswinney@amazon.com>
Cc: "Pop, Sebastian" <spop@amazon.com>,
"J. Dekker" <jdek@itanimul.li>, Hubert Mazur <hum@semihalf.com>,
"ffmpeg-devel@ffmpeg.org" <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH 1/1] libswscale/aarch64: add another hscale specialization
Date: Thu, 4 Aug 2022 14:30:29 +0300 (EEST)
Message-ID: <e1c88cb5-28f6-d5e6-4f21-a2e874332a73@martin.st> (raw)
In-Reply-To: <80e3a8b0fd7244e59cf543faa9896d7f@amazon.com>
On Fri, 22 Jul 2022, Swinney, Jonathan wrote:
> This specialization handles the case where filtersize is 4 mod 8, e.g.
> 12, 20, etc. Aarch64 was previously using the c function for this case.
> This implementation speeds up that case significantly.
>
> hscale_8_to_15__fs_12_dstW_512_c: 6234.1
> hscale_8_to_15__fs_12_dstW_512_neon: 1505.6
>
> Signed-off-by: Jonathan Swinney <jswinney@amazon.com>
> ---
> libswscale/aarch64/hscale.S | 107 +++++++++++++++++++++++++++++++++++
> libswscale/aarch64/swscale.c | 15 ++---
> 2 files changed, 115 insertions(+), 7 deletions(-)
>
> diff --git a/libswscale/aarch64/hscale.S b/libswscale/aarch64/hscale.S
> index b7b21b7a0f..93b9094ded 100644
> --- a/libswscale/aarch64/hscale.S
> +++ b/libswscale/aarch64/hscale.S
> @@ -91,6 +91,113 @@ function ff_hscale8to15_X8_neon, export=1
> ret
> endfunc
>
> +function ff_hscale8to15_X4_neon, export=1
> +// x0 SwsContext *c (not used)
> +// x1 int16_t *dst
> +// x2 int dstW
> +// x3 const uint8_t *src
> +// x4 const int16_t *filter
> +// x5 const int32_t *filterPos
> +// x6 int filterSize
Here, x2 and x6 should be w2 and w6
> +
> +// This function for filter sizes that are 4 mod 8. In other words, anything that's 0 mod 4 but not
> +// 0 mod 8. It also assumes that dstW is 0 mod 4.
> +
> + lsl w7, w6, #1 // w7 = filterSize * 2
> +1:
> + ldp w8, w9, [x5] // filterPos[idx + 0], [idx + 1]
> + ldp w10, w11, [x5, 8] // filterPos[idx + 2], [idx + 3]
With MS armasm64, this produces the following error:
libswscale\aarch64\hscale.o.asm(1034) : error A2079: improper line syntax;
symbol expected
ldp w10, w11, [x5, 8]
(The error is that the immediate offset should be written #8.)
> +
> + movi v16.2d, #0 // initialize accumulator for idx + 0
> + movi v17.2d, #0 // initialize accumulator for idx + 1
> + movi v18.2d, #0 // initialize accumulator for idx + 2
> + movi v19.2d, #0 // initialize accumulator for idx + 3
> +
> + mov x12, x4 // filter pointer for idx + 0
> + add x13, x4, x7 // filter pointer for idx + 1
> + add x8, x3, w8, uxtw // srcp + filterPos[idx + 0]
> + add x9, x3, w9, uxtw // srcp + filterPos[idx + 1]
> +
> + add x14, x13, x7 // filter pointer for idx + 2
> + add x10, x3, w10, uxtw // srcp + filterPos[idx + 2]
> + add x11, x3, w11, uxtw // srcp + filterPos[idx + 3]
> +
> + mov w0, w6 // copy filterSize to a temp register, w0
> + add x5, x5, #16 // advance the filterPos pointer
> + add x15, x14, x7 // filter pointer for idx + 3
> + mov x16, xzr // temp register for offsetting filter pointers
> +
> +2:
> + // This section loops over 8-wide chunks of filter size
> + ldr d4, [x8], #8 // load 8 bytes from srcp for idx + 0
> + ldr q0, [x12, x16] // load 8 values, 16 bytes from filter for idx + 0
> +
> + ldr d5, [x9], #8 // load 8 bytes from srcp for idx + 1
> + ldr q1, [x13, x16] // load 8 values, 16 bytes from filter for idx + 1
> +
> + uxtl v4.8h, v4.8b // unsigned extend long for idx + 0
> + uxtl v5.8h, v5.8b // unsigned extend long for idx + 1
> +
> + ldr d6, [x10], #8 // load 8 bytes from srcp for idx + 2
> + ldr q2, [x14, x16] // load 8 values, 16 bytes from filter for idx + 2
> +
> + smlal v16.4s, v0.4h, v4.4h // val += src[srcPos + j + 0..3] * filter[fs * i + j + 0..3], idx + 0
> + smlal v17.4s, v1.4h, v5.4h // val += src[srcPos + j + 0..3] * filter[fs * i + j + 0..3], idx + 1
> +
> + ldr d7, [x11], #8 // load 8 bytes from srcp for idx + 3
> + ldr q3, [x15, x16] // load 8 values, 16 bytes from filter for idx + 3
> +
> + sub w0, w0, #8 // decrement the remaining filterSize counter
> + smlal2 v16.4s, v0.8h, v4.8h // val += src[srcPos + j + 4..7] * filter[fs * i + j + 4..7], idx + 0
> + smlal2 v17.4s, v1.8h, v5.8h // val += src[srcPos + j + 4..7] * filter[fs * i + j + 4..7], idx + 1
> + uxtl v6.8h, v6.8b // unsigned extend long for idx + 2
> + uxtl v7.8h, v7.8b // unsigned extend long for idx + 3
> + smlal v18.4s, v2.4h, v6.4h // val += src[srcPos + j + 0..3] * filter[fs * i + j + 0..3], idx + 2
> + smlal v19.4s, v3.4h, v7.4h // val += src[srcPos + j + 0..3] * filter[fs * i + j + 0..3], idx + 3
> +
> + cmp w0, #8 // are there at least 8 more elements in filter to consume?
> + add x16, x16, #16 // advance the offsetting register for filter values
> +
> + smlal2 v18.4s, v2.8h, v6.8h // val += src[srcPos + j + 4..7] * filter[fs * i + j + 4..7], idx + 2
> + smlal2 v19.4s, v3.8h, v7.8h // val += src[srcPos + j + 4..7] * filter[fs * i + j + 4..7], idx + 3
> +
> + b.ge 2b // branch back to inner loop
> +
> + // complete the remaining 4 filter elements
> + sub x17, x7, #8 // calculate the offset of the filter pointer for the remaining 4 elements
> +
> + ldr s4, [x8] // load 4 bytes from srcp for idx + 0
> + ldr d0, [x12, x17] // load 4 values, 8 bytes from filter for idx + 0
> + ldr s5, [x9] // load 4 bytes from srcp for idx + 1
> + ldr d1, [x13, x17] // load 4 values, 8 bytes from filter for idx + 1
> +
> + uxtl v4.8h, v4.8b // unsigned extend long for idx + 0
> + uxtl v5.8h, v5.8b // unsigned extend long for idx + 1
> +
> + ldr s6, [x10] // load 4 bytes from srcp for idx + 2
> + ldr d2, [x14, x17] // load 4 values, 8 bytes from filter for idx + 2
> + smlal v16.4s, v0.4h, v4.4h // val += src[srcPos + j + 0..3] * filter[fs * i + j + 0..3], idx + 0
> + smlal v17.4s, v1.4h, v5.4h // val += src[srcPos + j + 0..3] * filter[fs * i + j + 0..3], idx + 1
> + ldr s7, [x11] // load 4 bytes from srcp for idx + 3
> + ldr d3, [x15, x17] // load 4 values, 8 bytes from filter for idx + 3
> +
> + uxtl v6.8h, v6.8b // unsigned extend long for idx + 2
> + uxtl v7.8h, v7.8b // unsigned extend long for idx + 3
> + addp v16.4s, v16.4s, v17.4s // horizontal pair adding for idx 0,1
> + smlal v18.4s, v2.4h, v6.4h // val += src[srcPos + j + 0..3] * filter[fs * i + j + 0..3], idx + 2
> + smlal v19.4s, v3.4h, v7.4h // val += src[srcPos + j + 0..3] * filter[fs * i + j + 0..3], idx + 3
> +
> + addp v18.4s, v18.4s, v19.4s // horizontal pair adding for idx 2,3
> + addp v16.4s, v16.4s, v18.4s // final horizontal pair adding producing one vector with results for idx = 0..3
> +
> + subs w2, w2, #4 // dstW -= 4
> + sqshrn v0.4h, v16.4s, #7 // shift and clip the 2x16-bit final values
> + st1 {v0.4h}, [x1], #8 // write to destination idx 0..3
> + add x4, x4, x7, lsl 2 // filter += (filterSize*2) * 4
This also fails to build with MS armasm64:
libswscale\aarch64\hscale.o.asm(1122) : error A2173: syntax error in
expression
add x4, x4, x7, lsl 2
Same issue here; it should be #2.
> #define ASSIGN_SCALE_FUNC(hscalefn, filtersize, opt) \
> - switch (filtersize) { \
> - case 4: ASSIGN_SCALE_FUNC2(hscalefn, 4, opt); break; \
> - default: if (filtersize % 8 == 0) \
> - ASSIGN_SCALE_FUNC2(hscalefn, X8, opt); \
> - break; \
> - }
> + if (filtersize == 4) \
> + ASSIGN_SCALE_FUNC2(hscalefn, 4, opt); \
> + else if (filtersize % 8 == 0) \
> + ASSIGN_SCALE_FUNC2(hscalefn, X8, opt); \
> + else if (filtersize % 4 == 0 && filtersize % 8 != 0) \
> + ASSIGN_SCALE_FUNC2(hscalefn, X4, opt);
>
Would it be safest to wrap this in an do { } while (0)?
Other than that, I think the patch seems fine to me!
// 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".
prev parent reply other threads:[~2022-08-04 11:30 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-22 2:02 Swinney, Jonathan
2022-08-04 11:30 ` Martin Storsjö [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e1c88cb5-28f6-d5e6-4f21-a2e874332a73@martin.st \
--to=martin@martin.st \
--cc=ffmpeg-devel@ffmpeg.org \
--cc=hum@semihalf.com \
--cc=jdek@itanimul.li \
--cc=jswinney@amazon.com \
--cc=spop@amazon.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git