Re: [libav-devel] [PATCH 07/14] vc1: Change type of array stride parameters to ptrdiff_t
On Thu, 29 Sep 2016, Diego Biurrun wrote: On Thu, Sep 29, 2016 at 02:24:32PM +0300, Martin Storsjö wrote: On Tue, 20 Sep 2016, Diego Biurrun wrote: > This avoids SIMD-optimized functions having to sign-extend their > stride argument manually to be able to do pointer arithmetic. > > Also rename all such parameters to "stride" for consistency. > --- > libavcodec/arm/vc1dsp_init_neon.c | 2 +- > libavcodec/vc1.c | 11 ++- > libavcodec/vc1_block.c| 21 + > libavcodec/vc1_loopfilter.c | 8 +--- > libavcodec/vc1_pred.c | 9 ++--- > libavcodec/vc1dsp.c | 26 +- > libavcodec/vc1dsp.h | 16 > libavcodec/x86/vc1dsp.asm | 22 +++--- > libavcodec/x86/vc1dsp_init.c | 16 > libavcodec/x86/vc1dsp_mmx.c | 21 +++-- > 10 files changed, 82 insertions(+), 70 deletions(-) > --- a/libavcodec/x86/vc1dsp.asm > +++ b/libavcodec/x86/vc1dsp.asm > @@ -237,19 +237,19 @@ cglobal vc1_h_loop_filter_internal > VC1_H_LOOP_FILTER 4, r4 > ret > > -; void ff_vc1_v_loop_filter4_mmxext(uint8_t *src, int stride, int pq) > +; void ff_vc1_v_loop_filter4_mmxext(uint8_t *src, ptrdiff_t stride, int pq) > cglobal vc1_v_loop_filter4, 3,5,0 > START_V_FILTER > call vc1_v_loop_filter_internal > RET I don't see the corresponding asm simplification as the commit message touts. I.e., this is probably a latent bug; fix that first with the proper sign extensions before scrambling things by changing the signature. I think I just used the wrong log message on this one. Changed locally to No, not really. There's three ways it can be: 1) The function actually doesn't use the stride parameter, and no change is needed 2) The function does use it correctly (e.g. doing a sign extension of it somewhere, or using it via register names like 'rNd' or so, making it explicitly that it's a 32 bit parameter). In those cases, we should most probably update the asm accordingly, i.e. remove the sign extension, or use 'rN' instead of 'rNd'. 3) The function doesn't use it correctly right now, and we have a bug that should be fixed before we change the type. In this case, I'm pretty sure that the parameter isn't unused in all those functions, that would be highly surprising. On a quick view, it seems like this parameter is used within the START_V/H_FILTER macros, as 'r1', which should probably be 'r1d' (or sign extended into r1 if one can't use r1d at those places - my x86 asm is very rusty). In general, when changing the type from int to ptrdiff_t, there should be a change in every single asm function where you change the signature. Otherwise the parameter is either unused, or there's a bug. I think. (If you've spent more time looking at it and can explain why this isn't the case, then please do.) // Martin ___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH 07/14] vc1: Change type of array stride parameters to ptrdiff_t
On Thu, Sep 29, 2016 at 02:24:32PM +0300, Martin Storsjö wrote: > On Tue, 20 Sep 2016, Diego Biurrun wrote: > > > This avoids SIMD-optimized functions having to sign-extend their > > stride argument manually to be able to do pointer arithmetic. > > > > Also rename all such parameters to "stride" for consistency. > > --- > > libavcodec/arm/vc1dsp_init_neon.c | 2 +- > > libavcodec/vc1.c | 11 ++- > > libavcodec/vc1_block.c| 21 + > > libavcodec/vc1_loopfilter.c | 8 +--- > > libavcodec/vc1_pred.c | 9 ++--- > > libavcodec/vc1dsp.c | 26 +- > > libavcodec/vc1dsp.h | 16 > > libavcodec/x86/vc1dsp.asm | 22 +++--- > > libavcodec/x86/vc1dsp_init.c | 16 > > libavcodec/x86/vc1dsp_mmx.c | 21 +++-- > > 10 files changed, 82 insertions(+), 70 deletions(-) > > --- a/libavcodec/x86/vc1dsp.asm > > +++ b/libavcodec/x86/vc1dsp.asm > > @@ -237,19 +237,19 @@ cglobal vc1_h_loop_filter_internal > > VC1_H_LOOP_FILTER 4, r4 > > ret > > > > -; void ff_vc1_v_loop_filter4_mmxext(uint8_t *src, int stride, int pq) > > +; void ff_vc1_v_loop_filter4_mmxext(uint8_t *src, ptrdiff_t stride, int pq) > > cglobal vc1_v_loop_filter4, 3,5,0 > > START_V_FILTER > > call vc1_v_loop_filter_internal > > RET > > I don't see the corresponding asm simplification as the commit message > touts. I.e., this is probably a latent bug; fix that first with the proper > sign extensions before scrambling things by changing the signature. I think I just used the wrong log message on this one. Changed locally to vc1: Change type of array stride parameters to ptrdiff_t ptrdiff_t is the correct type for array strides and similar. Also rename all such parameters to "stride" for consistency. Diego ___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
Re: [libav-devel] [PATCH 07/14] vc1: Change type of array stride parameters to ptrdiff_t
On Tue, 20 Sep 2016, Diego Biurrun wrote: This avoids SIMD-optimized functions having to sign-extend their stride argument manually to be able to do pointer arithmetic. Also rename all such parameters to "stride" for consistency. --- libavcodec/arm/vc1dsp_init_neon.c | 2 +- libavcodec/vc1.c | 11 ++- libavcodec/vc1_block.c| 21 + libavcodec/vc1_loopfilter.c | 8 +--- libavcodec/vc1_pred.c | 9 ++--- libavcodec/vc1dsp.c | 26 +- libavcodec/vc1dsp.h | 16 libavcodec/x86/vc1dsp.asm | 22 +++--- libavcodec/x86/vc1dsp_init.c | 16 libavcodec/x86/vc1dsp_mmx.c | 21 +++-- 10 files changed, 82 insertions(+), 70 deletions(-) diff --git a/libavcodec/x86/vc1dsp.asm b/libavcodec/x86/vc1dsp.asm index adf08d7..9136ad9 100644 --- a/libavcodec/x86/vc1dsp.asm +++ b/libavcodec/x86/vc1dsp.asm @@ -237,19 +237,19 @@ cglobal vc1_h_loop_filter_internal VC1_H_LOOP_FILTER 4, r4 ret -; void ff_vc1_v_loop_filter4_mmxext(uint8_t *src, int stride, int pq) +; void ff_vc1_v_loop_filter4_mmxext(uint8_t *src, ptrdiff_t stride, int pq) cglobal vc1_v_loop_filter4, 3,5,0 START_V_FILTER call vc1_v_loop_filter_internal RET I don't see the corresponding asm simplification as the commit message touts. I.e., this is probably a latent bug; fix that first with the proper sign extensions before scrambling things by changing the signature. // Martin ___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
