On Wed, 2 Nov 2016, Janne Grunau wrote:
On 2016-10-19 22:18:43 +0300, Martin Storsjö wrote:This work is sponsored by, and copyright, Google.The filter coefficients are signed values, where the product of the multiplication with one individual filter coefficient doesn't overflow a 16 bit signed value (the largest filter coefficient is 127). But when the products are accumulated, the resulting sum can overflow the 16 bit signed range. Instead of accumulating in 32 bit, we accumulate all filter taps but the largest one in one register, and the largest one (either index 3 or 4) in a separate one, added with saturation afterwards."Instead of ..., we accumulate the largest product (either index 3 or 4) last with a saturated addition." Is shorter and easier to understand.
Thanks, amended.
+#define define_8tap_2d_fn(op, filter, sz) \ +static void op##_##filter##sz##_hv_neon(uint8_t *dst, ptrdiff_t dst_stride, \ + const uint8_t *src, ptrdiff_t src_stride, \ + int h, int mx, int my) \ +{ \ + LOCAL_ALIGNED_16(uint8_t, temp, [72 * 64]); \Is there a reason why the temporary array doesn't depend on the size?
Mostly for simplicity. But I guess it is more efficient to not make it larger than necessary and using a tighter stride; fixed.
+ /* We only need h + 7 lines, but the horizontal filter assumes an \ + * even number of rows, so filter h + 8 lines here. */ \ + ff_vp9_put_##filter##sz##_h_neon(temp, 64, \ + src - 3 * src_stride, src_stride, \ + h + 8, mx, 0); \ + ff_vp9_##op##_##filter##sz##_v_neon(dst, dst_stride, \ + temp + 3 * 64, 64, \ + h, 0, my); \I guess you haven't tried implementing the diagonal mc fully in asm? Probably not worth the effort since the only significant gain comes from keeping the temporary values in registers which works on size 4 and 8 and maybe 16 for aarch64 wich are probably not that important.
I didn't try that fully, no. I did try an asm frontend just like this one, though, to avoid a separate vpush/vpop in each of them, and while it does save a little, I had to restructure the end of the filter function, which cost a little for the plain-horizontal/vertical functions. All in all I guess it would be a gain, but not really worth it I think. Being able to keep all the intermediates in registers would probably make it more worthwhile, but that would also be even more effort. Since you need 7 more rows than the actual input size, and the height can be width*2, I think it would be hard to keep the intermediates for anything more than size 4 in registers even if you'd do a handmade version for it.
+function ff_vp9_copy64_neon, export=1 + ldr r12, [sp] + sub r1, r1, #48 + sub r3, r3, #48 +1: + vld1.8 {q0}, [r2]! + vld1.8 {q1}, [r2]! + vst1.8 {q0}, [r0, :128]! + vld1.8 {q2}, [r2]! + vst1.8 {q1}, [r0, :128]! + vld1.8 {q3}, [r2], r3 + vst1.8 {q2}, [r0, :128]! + subs r12, r12, #1 + vst1.8 {q3}, [r0, :128], r1 + bne 1b + bx lr +endfunc + +function ff_vp9_avg64_neon, export=1 + ldr r12, [sp] + sub r1, r1, #48 + sub r3, r3, #48 +1: + vld1.8 {q8}, [r2]! + vld1.8 {q0}, [r0, :128]! + vld1.8 {q9}, [r2]! + vld1.8 {q1}, [r0, :128]! + vrhadd.u8 q0, q0, q8 + vld1.8 {q10}, [r2]! + vld1.8 {q2}, [r0, :128]! + vrhadd.u8 q1, q1, q9 + vld1.8 {q11}, [r2], r3 + vld1.8 {q3}, [r0, :128] + vrhadd.u8 q2, q2, q10 + sub r0, r0, #48using an additional register for writing to dst proabably makes sense
Yes, that helped a little, to avoid the rewind of r0 here
+ vrhadd.u8 q3, q3, q11 + vst1.8 {q0}, [r0, :128]! + vst1.8 {q1}, [r0, :128]! + vst1.8 {q2}, [r0, :128]! + vst1.8 {q3}, [r0, :128], r1 + subs r12, r12, #1 + bne 1b + bx lr +endfuncHave you tried 4 d-register loads/stores? If it's not slower it's preferable since it uses less instructions. Same for size 32 with the additional advantange of not having to adjust the stride
Oh, why didn't I remember those instructions? Yes, this gives a huge gain, like a 20% speedup in the 32/64 functions. Thanks!
+ +function ff_vp9_copy32_neon, export=1 + ldr r12, [sp] + sub r1, r1, #16 + sub r3, r3, #16 +1: + vld1.8 {q0}, [r2]! + vst1.8 {q0}, [r0, :128]! + subs r12, r12, #1 + vld1.8 {q1}, [r2], r3 + vst1.8 {q1}, [r0, :128], r1 + bne 1b + bx lr +endfunc + +function ff_vp9_avg32_neon, export=1 + ldr r12, [sp] + sub r1, r1, #16 + sub r3, r3, #16 +1: + vld1.8 {q2}, [r2]! + vld1.8 {q0}, [r0, :128]! + vld1.8 {q3}, [r2], r3 + vrhadd.u8 q0, q0, q2 + vld1.8 {q1}, [r0, :128] + vrhadd.u8 q1, q1, q3 + sub r0, r0, #1632-byte load for dst will leave r0 unmodified
Yep, immensely helpful!
+ vst1.8 {q0}, [r0, :128]! + vst1.8 {q1}, [r0, :128], r1 + subs r12, r12, #1 + bne 1b + bx lr +endfunc + +function ff_vp9_copy16_neon, export=1 + ldr r12, [sp] + push {r4-r5} + add r4, r0, r1 + add r5, r2, r3 + add r1, r1, r1 + add r3, r3, r3 +1: + vld1.8 {q0}, [r2], r3 + vld1.8 {q1}, [r5], r3 + subs r12, r12, #2 + vst1.8 {q0}, [r0, :128], r1 + vst1.8 {q1}, [r4, :128], r1 + bne 1b + pop {r4-r5} + bx lr +endfunc + +function ff_vp9_avg16_neon, export=1 + ldr r12, [sp] +1: + vld1.8 {q2}, [r2], r3 + vld1.8 {q0}, [r0, :128], r1 + vld1.8 {q3}, [r2], r3 + vrhadd.u8 q0, q0, q2 + vld1.8 {q1}, [r0, :128] + sub r0, r0, r1have you tried storing q0 with writeback and then loading q1?
I tried it now, but it gave a very minor speedup on two of the tested cores, and an equally large slowdown on the other ones, so it doesn't seem to be useful.
+ vrhadd.u8 q1, q1, q3 + subs r12, r12, #2 + vst1.8 {q0}, [r0, :128], r1 + vst1.8 {q1}, [r0, :128], r1 + bne 1b + bx lr +endfunc + +function ff_vp9_copy8_neon, export=1 + ldr r12, [sp] +1: + vld1.8 {d0}, [r2], r3 + vld1.8 {d1}, [r2], r3 + subs r12, r12, #2 + vst1.8 {d0}, [r0, :64], r1 + vst1.8 {d1}, [r0, :64], r1 + bne 1b + bx lr +endfunc + +function ff_vp9_avg8_neon, export=1 + ldr r12, [sp] +1: + vld1.8 {d2}, [r2], r3 + vld1.8 {d0}, [r0, :64], r1 + vld1.8 {d3}, [r2], r3 + vrhadd.u8 d0, d0, d2 + vld1.8 {d1}, [r0, :64] + sub r0, r0, r1same as 16 although it's probably silly for size 8, using an additional register would the other possibility to speed it up
Didn't turn out to help on more than 1 core actually, and gave an equal slowdown on the others
+.macro do_8tap_h_size size +do_8tap_h put, \size, 3, 4 +do_8tap_h avg, \size, 3, 4 +do_8tap_h put, \size, 4, 3 +do_8tap_h avg, \size, 4, 3 +.endm + +do_8tap_h_size 4 +do_8tap_h_size 8 +do_8tap_h_size 16 +do_8tap_h_size 32I'm not sure if it's worth to have the size 32+ version. The additional loop checks are probably not noticeable in size 16 case and it would reduce the binary size.
Fair point. This reduced the code size for this object file from 13308 bytes to 11724 bytes, which is kinda significant.
The other idea for keeping the calculation in signed 16-bit would have been using a negative constant as offset since the range doesn't exceed 16-bit. Not faster in checkasm but smaller binary size. No need to try that. Current implementation is fine for me and looks quite nice.
Thanks!
+@ Evaluate the filter twice in parallel, from the inputs src1-src9 into dst1-dst2 +@ (src1-src8 into dst1, src2-src9 into dst2), adding idx2 separately +@ at the end with saturation +.macro convolve dst1, dst2, src1, src2, src3, src4, src5, src6, src7, src8, src9, idx1, idx2, tmp1, tmp2 + vmul.s16 \dst1, \src1, d0[0] + vmul.s16 \dst2, \src2, d0[0] + vmla.s16 \dst1, \src2, d0[1] + vmla.s16 \dst2, \src3, d0[1] + vmla.s16 \dst1, \src3, d0[2] + vmla.s16 \dst2, \src4, d0[2] +.if \idx1 == 3 + vmla.s16 \dst1, \src4, d0[3] + vmla.s16 \dst2, \src5, d0[3] +.else + vmla.s16 \dst1, \src5, d1[0] + vmla.s16 \dst2, \src6, d1[0] +.endif + vmla.s16 \dst1, \src6, d1[1] + vmla.s16 \dst2, \src7, d1[1] + vmla.s16 \dst1, \src7, d1[2] + vmla.s16 \dst2, \src8, d1[2] + vmla.s16 \dst1, \src8, d1[3] + vmla.s16 \dst2, \src9, d1[3] +.if \idx2 == 3 + vmul.s16 \tmp1, \src4, d0[3] + vmul.s16 \tmp2, \src5, d0[3] +.else + vmul.s16 \tmp1, \src5, d1[0] + vmul.s16 \tmp2, \src6, d1[0] +.endif + vqadd.s16 \dst1, \dst1, \tmp1 + vqadd.s16 \dst2, \dst2, \tmp2 +.endmthe negative terms could be accumulated in tmp1/2 I doubt it makes a difference in practice but reduces data dependencies.
This did actually turn out to help quite a lot on A53 (from 11707 cycles to 9649, for the 64v function). Very marginal slowdowns on A8 and A9 though, but clearly worthwhile doing.
...+@ Instantiate a vertical filter function for filtering a 4 pixels wide +@ slice. The first half of the registers contain one row, while the second +@ half of a register contains the second-next row (also stored in the first +@ half of the register two steps ahead). The convolution does two outputs +@ at a time; the output of q5-q12 into one, and q4-q13 into another one. +@ The first half of first output is the first output row, the first half +@ of the other output is the second output row. The second halves of the +@ registers are rows 3 and 4. +@ This only is designed to work for 4 or 8 output lines. +.macro do_8tap_4v type, idx1, idx2 +function \type\()_8tap_4v_\idx1\idx2 + sub r2, r2, r3, lsl #1 + sub r2, r2, r3 + vld1.16 {q0}, [r12, :128] + + vld1.32 {d2[]}, [r2], r3 + vld1.32 {d3[]}, [r2], r3 + vld1.32 {d4[]}, [r2], r3 + vld1.32 {d5[]}, [r2], r3 + vld1.32 {d6[]}, [r2], r3 + vld1.32 {d7[]}, [r2], r3 + vld1.32 {d8[]}, [r2], r3 + vld1.32 {d9[]}, [r2], r3 + vld1.32 {d28[]}, [r2] + sub r2, r2, r3, lsl #2 + sub r2, r2, r3, lsl #1 + vld1.32 {d2[1]}, [r2], r3vext.8 d2, d2, d4, #4 should be faster than loading. it works since we loaded data to all lanes
Oh, clever! This was also quite helpful, especially since this function is pretty short and fast anyway, so the change is quite noticeable.
+ vld1.32 {d3[1]}, [r2], r3 + vmovl.u8 q5, d2 + vld1.32 {d4[1]}, [r2], r3 + vmovl.u8 q6, d3 + vld1.32 {d5[1]}, [r2], r3 + vmovl.u8 q7, d4 + vld1.32 {d6[1]}, [r2], r3 + vmovl.u8 q8, d5 + vld1.32 {d7[1]}, [r2], r3 + vmovl.u8 q9, d6 + vld1.32 {d8[1]}, [r2], r3 + vmovl.u8 q10, d7 + vld1.32 {d9[1]}, [r2], r3 + vmovl.u8 q11, d8 + vld1.32 {d28[1]}, [r2] + sub r2, r2, r3 + + vmovl.u8 q12, d9 + vmovl.u8 q13, d28 + + convolve q1, q2, q5, q6, q7, q8, q9, q10, q11, q12, q13, \idx1, \idx2, q4, q3 + do_store4 q1, d2, q2, d4, d3, d5, \type + subs r4, r4, #4 + beq 9f + + vld1.32 {d2[]}, [r2], r3 + vld1.32 {d3[]}, [r2], r3 + vld1.32 {d4[]}, [r2], r3 + vld1.32 {d5[]}, [r2], r3 + sub r2, r2, r3, lsl #1 + vld1.32 {d2[1]}, [r2], r3 + vld1.32 {d3[1]}, [r2], r3 + vmovl.u8 q14, d2 + vld1.32 {d4[1]}, [r2], r3 + vmovl.u8 q15, d3 + vld1.32 {d5[1]}, [r2], r3 + vmovl.u8 q5, d4 + vmovl.u8 q6, d5 + + convolve q1, q2, q9, q10, q11, q12, q13, q14, q15, q5, q6, \idx1, \idx2, q4, q3 + do_store4 q1, d2, q2, d4, d3, d5, \type + +9: + vpop {q4-q7} + pop {r4-r5} + bx lr +endfunc +.endm + +do_8tap_4v put, 3, 4 +do_8tap_4v put, 4, 3 +do_8tap_4v avg, 3, 4 +do_8tap_4v avg, 4, 3 + +.macro do_8tap_v_func type, filter, size +function ff_vp9_\type\()_\filter\()\size\()_v_neon, export=1 + push {r4-r5} + vpush {q4-q7} + ldr r4, [sp, #72] + ldr r5, [sp, #80] + movrel r12, \filter\()_filter-16 + add r12, r12, r5, lsl #4 + cmp r5, #8 + mov r5, #\sizea little confusing that this changes r5 and r12 compared to the horizontal mc, changing the horizontal mc so that r5 holds the width and r12 the filter constants/tmp reg would work if you care
Indeed; I changed that and now it's consistent.Thanks for the review! I'll post the updated patch, and apply the same review comments to the aarch64 version and repost that one as soon as I get them applied there.
// Martin _______________________________________________ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
