| Message ID | 20170405015328.5476-2-jdarnley@obe.tv |
|---|---|
| State | Superseded |
| Headers | show |
On 4/4/2017 10:53 PM, James Darnley wrote: > --- > libavcodec/x86/h264_idct.asm | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/libavcodec/x86/h264_idct.asm b/libavcodec/x86/h264_idct.asm > index c36fea5..878ff02 100644 > --- a/libavcodec/x86/h264_idct.asm > +++ b/libavcodec/x86/h264_idct.asm > @@ -695,7 +695,7 @@ cglobal h264_idct_add8_8, 5, 8 + npicregs, 0, dst1, block_offset, block, stride, > add r0mp, gprsize > %endif > call h264_idct_add8_mmx_plane > - RET > + RET ; TODO: check rep ret after a function call > > cglobal h264_idct_add8_422_8, 5, 8 + npicregs, 0, dst1, block_offset, block, stride, nnzc, cntr, coeff, dst2, picreg > ; dst1, block_offset, block, stride, nnzc, cntr, coeff, dst2, picreg > @@ -727,7 +727,7 @@ cglobal h264_idct_add8_422_8, 5, 8 + npicregs, 0, dst1, block_offset, block, str > add r5, 4 > call h264_idct_add8_mmx_plane > > - RET > + RET ; TODO: check rep ret after a function call > > h264_idct_add8_mmxext_plane: > movsxdifnidn r3, r3d > @@ -795,7 +795,7 @@ cglobal h264_idct_add8_8, 5, 8 + npicregs, 0, dst1, block_offset, block, stride, > add r0mp, gprsize > %endif > call h264_idct_add8_mmxext_plane > - RET > + RET ; TODO: check rep ret after a function call > > ; r0 = uint8_t *dst, r2 = int16_t *block, r3 = int stride, r6=clobbered > h264_idct_dc_add8_mmxext: > @@ -878,7 +878,7 @@ cglobal h264_idct_add16_8, 5, 5 + ARCH_X86_64, 8 > add16_sse2_cycle 5, 0x24 > add16_sse2_cycle 6, 0x1e > add16_sse2_cycle 7, 0x26 > - RET > +REP_RET > > %macro add16intra_sse2_cycle 2 > movzx r0, word [r4+%2] > @@ -925,7 +925,7 @@ cglobal h264_idct_add16intra_8, 5, 7 + ARCH_X86_64, 8 > add16intra_sse2_cycle 5, 0x24 > add16intra_sse2_cycle 6, 0x1e > add16intra_sse2_cycle 7, 0x26 > - RET > +REP_RET > > %macro add8_sse2_cycle 2 > movzx r0, word [r4+%2] > @@ -980,7 +980,7 @@ cglobal h264_idct_add8_8, 5, 7 + ARCH_X86_64, 8 > %endif > add8_sse2_cycle 2, 0x5c > add8_sse2_cycle 3, 0x64 > - RET > +REP_RET > > ;void ff_h264_luma_dc_dequant_idct_mmx(int16_t *output, int16_t *input, int qmul) This is not necessary. Look at the RET macro in x86inc, It calls the AUTO_REP_RET macro.
On 2017-04-05 05:33, James Almer wrote: > On 4/4/2017 10:53 PM, James Darnley wrote: >> --- >> libavcodec/x86/h264_idct.asm | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/libavcodec/x86/h264_idct.asm b/libavcodec/x86/h264_idct.asm >> index c36fea5..878ff02 100644 >> --- a/libavcodec/x86/h264_idct.asm >> +++ b/libavcodec/x86/h264_idct.asm >> @@ -695,7 +695,7 @@ cglobal h264_idct_add8_8, 5, 8 + npicregs, 0, dst1, block_offset, block, stride, >> add r0mp, gprsize >> %endif >> call h264_idct_add8_mmx_plane >> - RET >> + RET ; TODO: check rep ret after a function call >> >> cglobal h264_idct_add8_422_8, 5, 8 + npicregs, 0, dst1, block_offset, block, stride, nnzc, cntr, coeff, dst2, picreg >> ; dst1, block_offset, block, stride, nnzc, cntr, coeff, dst2, picreg >> @@ -727,7 +727,7 @@ cglobal h264_idct_add8_422_8, 5, 8 + npicregs, 0, dst1, block_offset, block, str >> add r5, 4 >> call h264_idct_add8_mmx_plane >> >> - RET >> + RET ; TODO: check rep ret after a function call >> >> h264_idct_add8_mmxext_plane: >> movsxdifnidn r3, r3d >> @@ -795,7 +795,7 @@ cglobal h264_idct_add8_8, 5, 8 + npicregs, 0, dst1, block_offset, block, stride, >> add r0mp, gprsize >> %endif >> call h264_idct_add8_mmxext_plane >> - RET >> + RET ; TODO: check rep ret after a function call >> >> ; r0 = uint8_t *dst, r2 = int16_t *block, r3 = int stride, r6=clobbered >> h264_idct_dc_add8_mmxext: >> @@ -878,7 +878,7 @@ cglobal h264_idct_add16_8, 5, 5 + ARCH_X86_64, 8 >> add16_sse2_cycle 5, 0x24 >> add16_sse2_cycle 6, 0x1e >> add16_sse2_cycle 7, 0x26 >> - RET >> +REP_RET >> >> %macro add16intra_sse2_cycle 2 >> movzx r0, word [r4+%2] >> @@ -925,7 +925,7 @@ cglobal h264_idct_add16intra_8, 5, 7 + ARCH_X86_64, 8 >> add16intra_sse2_cycle 5, 0x24 >> add16intra_sse2_cycle 6, 0x1e >> add16intra_sse2_cycle 7, 0x26 >> - RET >> +REP_RET >> >> %macro add8_sse2_cycle 2 >> movzx r0, word [r4+%2] >> @@ -980,7 +980,7 @@ cglobal h264_idct_add8_8, 5, 7 + ARCH_X86_64, 8 >> %endif >> add8_sse2_cycle 2, 0x5c >> add8_sse2_cycle 3, 0x64 >> - RET >> +REP_RET >> >> ;void ff_h264_luma_dc_dequant_idct_mmx(int16_t *output, int16_t *input, int qmul) > > This is not necessary. Look at the RET macro in x86inc, It calls the AUTO_REP_RET > macro. As I said last time, the macro only knows when the previous instruction was a jump, not when ret is a branch target. These macros contain a jump to a label at the end which means we should use REP_RET. The relevant doc comment: > ; On AMD cpus <=K10, an ordinary ret is slow if it immediately follows either > ; a branch or a branch target. So switch to a 2-byte form of ret in that case. > ; We can automatically detect "follows a branch", but not a branch target. > ; (SSSE3 is a sufficient condition to know that your cpu doesn't have this problem.) Has x86inc gained more magic? I know people don't care much about old CPUs, I hardly do either. That said I do plan to resurrect an old K8 (if I ever tidy this place) just so I have an sse2slow machine available.
On Wed, Apr 5, 2017 at 3:53 AM, James Darnley <jdarnley@obe.tv> wrote: > call h264_idct_add8_mmx_plane > - RET > + RET ; TODO: check rep ret after a function call call followed by RET should be replaced by the TAIL_CALL macro instead which outputs a jmp instruction if there's no function epilogue.
On 2017-04-05 22:26, Henrik Gramner wrote: > On Wed, Apr 5, 2017 at 3:53 AM, James Darnley <jdarnley@obe.tv> wrote: >> call h264_idct_add8_mmx_plane >> - RET >> + RET ; TODO: check rep ret after a function call > > call followed by RET should be replaced by the TAIL_CALL macro instead > which outputs a jmp instruction if there's no function epilogue. Do you want me to change this patch to add that?
On Fri, Apr 14, 2017 at 1:19 PM, James Darnley <jdarnley@obe.tv> wrote:
> Do you want me to change this patch to add that?
Either the same patch or a different one, pick whichever is most
convenient for you.
diff --git a/libavcodec/x86/h264_idct.asm b/libavcodec/x86/h264_idct.asm index c36fea5..878ff02 100644 --- a/libavcodec/x86/h264_idct.asm +++ b/libavcodec/x86/h264_idct.asm @@ -695,7 +695,7 @@ cglobal h264_idct_add8_8, 5, 8 + npicregs, 0, dst1, block_offset, block, stride, add r0mp, gprsize %endif call h264_idct_add8_mmx_plane - RET + RET ; TODO: check rep ret after a function call cglobal h264_idct_add8_422_8, 5, 8 + npicregs, 0, dst1, block_offset, block, stride, nnzc, cntr, coeff, dst2, picreg ; dst1, block_offset, block, stride, nnzc, cntr, coeff, dst2, picreg @@ -727,7 +727,7 @@ cglobal h264_idct_add8_422_8, 5, 8 + npicregs, 0, dst1, block_offset, block, str add r5, 4 call h264_idct_add8_mmx_plane - RET + RET ; TODO: check rep ret after a function call h264_idct_add8_mmxext_plane: movsxdifnidn r3, r3d @@ -795,7 +795,7 @@ cglobal h264_idct_add8_8, 5, 8 + npicregs, 0, dst1, block_offset, block, stride, add r0mp, gprsize %endif call h264_idct_add8_mmxext_plane - RET + RET ; TODO: check rep ret after a function call ; r0 = uint8_t *dst, r2 = int16_t *block, r3 = int stride, r6=clobbered h264_idct_dc_add8_mmxext: @@ -878,7 +878,7 @@ cglobal h264_idct_add16_8, 5, 5 + ARCH_X86_64, 8 add16_sse2_cycle 5, 0x24 add16_sse2_cycle 6, 0x1e add16_sse2_cycle 7, 0x26 - RET +REP_RET %macro add16intra_sse2_cycle 2 movzx r0, word [r4+%2] @@ -925,7 +925,7 @@ cglobal h264_idct_add16intra_8, 5, 7 + ARCH_X86_64, 8 add16intra_sse2_cycle 5, 0x24 add16intra_sse2_cycle 6, 0x1e add16intra_sse2_cycle 7, 0x26 - RET +REP_RET %macro add8_sse2_cycle 2 movzx r0, word [r4+%2] @@ -980,7 +980,7 @@ cglobal h264_idct_add8_8, 5, 7 + ARCH_X86_64, 8 %endif add8_sse2_cycle 2, 0x5c add8_sse2_cycle 3, 0x64 - RET +REP_RET ;void ff_h264_luma_dc_dequant_idct_mmx(int16_t *output, int16_t *input, int qmul)