Hello Tanu,
thank you for reviewing the code, my comments are inline
> > use (1<<15) instead of 0x7fff as a factor when converting from s16 to float32
> > use (1<<31) instead of 0x7fffffff as a factor when converting from s32 to float32
> > Signed-off-by: Peter Meerwald <p.meerwald at bct-electronic.com>
> > ---
> > src/pulsecore/sconv-s16le.c | 84 +++++++++++++++++++------------------------
> > src/pulsecore/sconv_neon.c | 17 ++++-----
> > src/pulsecore/sconv_sse.c | 66 +++++++++++++++++-----------------
> > 3 files changed, 75 insertions(+), 92 deletions(-)
> >
> > diff --git a/src/pulsecore/sconv-s16le.c b/src/pulsecore/sconv-s16le.c
> > index 138e418..e0d7975 100644
> > --- a/src/pulsecore/sconv-s16le.c
> > +++ b/src/pulsecore/sconv-s16le.c
> > @@ -85,11 +85,11 @@ void pa_sconv_s16le_to_float32ne(unsigned n, const int16_t *a, float *b) {
> > #if SWAP_WORDS == 1
> > for (; n > 0; n--) {
> > int16_t s = *(a++);
> > - *(b++) = ((float) INT16_FROM(s))/(float) 0x7FFF;
> > + *(b++) = INT16_FROM(s) * (1.0f / (1 << 15));
> > }
> > #else
> > for (; n > 0; n--)
> > - *(b++) = ((float) (*(a++)))/(float) 0x7FFF;
> > + *(b++) = *(a++) * (1.0f / (1 << 15));
> > #endif
> > }
> >
> > @@ -100,11 +100,11 @@ void pa_sconv_s32le_to_float32ne(unsigned n, const int32_t *a, float *b) {
> > #if SWAP_WORDS == 1
> > for (; n > 0; n--) {
> > int32_t s = *(a++);
> > - *(b++) = (float) (((double) INT32_FROM(s))/0x7FFFFFFF);
> > + *(b++) = INT32_FROM(s) * (1.0f / (1U << 31));
> > }
> > #else
> > for (; n > 0; n--)
> > - *(b++) = (float) (((double) (*(a++)))/0x7FFFFFFF);
> > + *(b++) = *(a++) * (1.0f / (1U << 31));
> > #endif
> > }
> >
> > @@ -115,18 +115,16 @@ void pa_sconv_s16le_from_float32ne(unsigned n, const float *a, int16_t *b) {
> > #if SWAP_WORDS == 1
> > for (; n > 0; n--) {
> > int16_t s;
> > - float v = *(a++);
> > + float v = *(a++) * (1 << 15);
> >
> > - v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.f);
> > - s = (int16_t) lrintf(v * 0x7FFF);
> > + s = (int16_t) PA_CLAMP_UNLIKELY(lrintf(v), -0x8000, 0x7FFF);
> If you call lrintf() before clamping the input, you get undefined
> results in case the input is very large. I guess this would work:
true; but this is on purpose to avoid the explicit clamping
the aim is to use only implicit clamping (with saturation) when narrowing
the data type from long int to int16_t -- and it turns out that x86 and NEON have
suitable instructions for that
'large input' would be > LONG_MAX (or < LONG_MIN) -- so there is quite a
bit of 'headroom' between normally values around 1<<15, and 1<<31 (on
32-bit systems) or 1<<63 (on 64-bit systems)
> s = (int16_t) lrintf(PA_CLAMP_UNLIKELY(v, (float) -0x8000, (float) 0x7FFF));
> The same comment applies to all "from float" conversions.
we can of course do the explicit clamping in C and tolerate deviating
results for large inputs in optimized implementations -- some other code
is seriously broken if the input is huge floats
> > *(b++) = INT16_TO(s);
> > }
> > #else
> > for (; n > 0; n--) {
> > - float v = *(a++);
> > + float v = *(a++) * (1 << 15);
> >
> > - v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.f);
> > - *(b++) = (int16_t) lrintf(v * 0x7FFF);
> > + *(b++) = (int16_t) PA_CLAMP_UNLIKELY(lrintf(v), -0x8000, 0x7FFF);
> > }
> > #endif
> > }
> > @@ -138,18 +136,16 @@ void pa_sconv_s32le_from_float32ne(unsigned n, const float *a, int32_t *b) {
> > #if SWAP_WORDS == 1
> > for (; n > 0; n--) {
> > int32_t s;
> > - float v = *(a++);
> > + float v = *(a++) * (1U << 31);
> >
> > - v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.0f);
> > - s = (int32_t) lrint((double) v * (double) 0x7FFFFFFF);
> > + s = (int32_t) PA_CLAMP_UNLIKELY(lrintf(v), -0x80000000LL, 0x7FFFFFFFLL);
>
> You probably meant to use llrintf() rather than lrintf(), but if you do
> the clamping before the rounding, I guess lrintf() will be enough here
> and in the other "from float" functions.
good catch; should be llrintf()
> > *(b++) = INT32_TO(s);
> > }
> > #else
> > for (; n > 0; n--) {
> > - float v = *(a++);
> > + float v = *(a++) * (1U << 31);
> >
> > - v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.0f);
> > - *(b++) = (int32_t) lrint((double) v * (double) 0x7FFFFFFF);
> > + *(b++) = (int32_t) PA_CLAMP_UNLIKELY(lrintf(v), -0x80000000LL, 0x7FFFFFFFLL);
> > }
> > #endif
> > }
> > @@ -160,7 +156,7 @@ void pa_sconv_s16le_to_float32re(unsigned n, const int16_t *a, float *b) {
> >
> > for (; n > 0; n--) {
> > int16_t s = *(a++);
> > - float k = ((float) INT16_FROM(s))/0x7FFF;
> > + float k = INT16_FROM(s) * (1.0f / (1 << 15));
> > k = PA_FLOAT32_SWAP(k);
> > *(b++) = k;
> > }
> > @@ -172,7 +168,7 @@ void pa_sconv_s32le_to_float32re(unsigned n, const int32_t *a, float *b) {
> >
> > for (; n > 0; n--) {
> > int32_t s = *(a++);
> > - float k = (float) (((double) INT32_FROM(s))/0x7FFFFFFF);
> > + float k = INT32_FROM(s) * (1.0f / (1U << 31));
> > k = PA_FLOAT32_SWAP(k);
> > *(b++) = k;
> > }
> > @@ -185,9 +181,8 @@ void pa_sconv_s16le_from_float32re(unsigned n, const float *a, int16_t *b) {
> > for (; n > 0; n--) {
> > int16_t s;
> > float v = *(a++);
> > - v = PA_FLOAT32_SWAP(v);
> > - v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.0f);
> > - s = (int16_t) lrintf(v * 0x7FFF);
> > + v = PA_FLOAT32_SWAP(v) * (1 << 15);
> > + s = (int16_t) PA_CLAMP_UNLIKELY(lrintf(v), -0x8000, 0x7FFF);
> > *(b++) = INT16_TO(s);
> > }
> > }
> > @@ -199,9 +194,8 @@ void pa_sconv_s32le_from_float32re(unsigned n, const float *a, int32_t *b) {
> > for (; n > 0; n--) {
> > int32_t s;
> > float v = *(a++);
> > - v = PA_FLOAT32_SWAP(v);
> > - v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.0f);
> > - s = (int32_t) lrint((double) v * 0x7FFFFFFF);
> > + v = PA_FLOAT32_SWAP(v) * (1U << 31);
> > + s = (int32_t) PA_CLAMP_UNLIKELY(llrintf(v), -0x80000000LL, 0x7FFFFFFFLL);
> > *(b++) = INT32_TO(s);
> > }
> > }
> > @@ -304,9 +298,9 @@ void pa_sconv_s24le_to_float32ne(unsigned n, const uint8_t *a, float *b) {
> >
> > for (; n > 0; n--) {
> > int32_t s = READ24(a) << 8;
> > - *b = ((float) s) / 0x7FFFFFFF;
> > + *b = s * (1.0f / (1U << 31));
> > a += 3;
> > - b ++;
> > + b++;
> > }
> > }
> >
> > @@ -316,12 +310,11 @@ void pa_sconv_s24le_from_float32ne(unsigned n, const float *a, uint8_t *b) {
> >
> > for (; n > 0; n--) {
> > int32_t s;
> > - float v = *a;
> > - v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.0f);
> > - s = (int32_t) lrint((double) v * (double) 0x7FFFFFFF);
> > + float v = *a * (1U << 31);
> > + s = (int32_t) PA_CLAMP_UNLIKELY(llrint(v), -0x80000000LL, 0x7FFFFFFFLL);
> > WRITE24(b, ((uint32_t) s) >> 8);
> > a++;
> > - b+=3;
> > + b += 3;
> > }
> > }
> >
> > @@ -331,10 +324,10 @@ void pa_sconv_s24le_to_float32re(unsigned n, const uint8_t *a, float *b) {
> >
> > for (; n > 0; n--) {
> > int32_t s = READ24(a) << 8;
> > - float k = ((float) s) / 0x7FFFFFFF;
> > + float k = s * (1.0f / (1U << 31));
> > *b = PA_FLOAT32_SWAP(k);
> > a += 3;
> > - b ++;
> > + b++;
> > }
> > }
> >
> > @@ -345,9 +338,8 @@ void pa_sconv_s24le_from_float32re(unsigned n, const float *a, uint8_t *b) {
> > for (; n > 0; n--) {
> > int32_t s;
> > float v = *a;
> > - v = PA_FLOAT32_SWAP(v);
> > - v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.0f);
> > - s = (int32_t) lrint((double) v * (double) 0x7FFFFFFF);
> > + v = PA_FLOAT32_SWAP(v) * (1U << 31);
> > + s = (int32_t) PA_CLAMP_UNLIKELY(llrint(v), -0x80000000LL, 0x7FFFFFFFLL);
> > WRITE24(b, ((uint32_t) s) >> 8);
> > a++;
> > b+=3;
> > @@ -406,9 +398,9 @@ void pa_sconv_s24_32le_to_float32ne(unsigned n, const uint32_t *a, float *b) {
> >
> > for (; n > 0; n--) {
> > int32_t s = (int32_t) (UINT32_FROM(*a) << 8);
> > - *b = (float) s / (float) 0x7FFFFFFF;
> > - a ++;
> > - b ++;
> > + *b = s * (1.0f / (1U << 31));
> > + a++;
> > + b++;
> > }
> > }
> >
> > @@ -418,10 +410,10 @@ void pa_sconv_s24_32le_to_float32re(unsigned n, const uint32_t *a, float *b) {
> >
> > for (; n > 0; n--) {
> > int32_t s = (int32_t) (UINT32_FROM(*a) << 8);
> > - float k = (float) s / (float) 0x7FFFFFFF;
> > + float k = s * (1.0f / (1U << 31));
> > *b = PA_FLOAT32_SWAP(k);
> > - a ++;
> > - b ++;
> > + a++;
> > + b++;
> > }
> > }
> >
> > @@ -431,9 +423,8 @@ void pa_sconv_s24_32le_from_float32ne(unsigned n, const float *a, uint32_t *b) {
> >
> > for (; n > 0; n--) {
> > int32_t s;
> > - float v = *a;
> > - v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.0f);
> > - s = (int32_t) lrint((double) v * (double) 0x7FFFFFFF);
> > + float v = *a * (1U << 31);
> > + s = (int32_t) PA_CLAMP_UNLIKELY(llrint(v), -0x80000000LL, 0x7FFFFFFFLL);
> > *b = UINT32_TO(((uint32_t) s) >> 8);
> > a++;
> > b++;
> > @@ -447,9 +438,8 @@ void pa_sconv_s24_32le_from_float32re(unsigned n, const float *a, uint32_t *b) {
> > for (; n > 0; n--) {
> > int32_t s;
> > float v = *a;
> > - v = PA_FLOAT32_SWAP(v);
> > - v = PA_CLAMP_UNLIKELY(v, -1.0f, 1.0f);
> > - s = (int32_t) lrint((double) v * (double) 0x7FFFFFFF);
> > + v = PA_FLOAT32_SWAP(v) * (1U << 31);
> > + s = (int32_t) PA_CLAMP_UNLIKELY(llrint(v), -0x80000000LL, 0x7FFFFFFFLL);
> > *b = UINT32_TO(((uint32_t) s) >> 8);
> > a++;
> > b++;
> > diff --git a/src/pulsecore/sconv_neon.c b/src/pulsecore/sconv_neon.c
> > index 6fd966d..111b56f 100644
> > --- a/src/pulsecore/sconv_neon.c
> > +++ b/src/pulsecore/sconv_neon.c
> > @@ -36,16 +36,11 @@ static void pa_sconv_s16le_from_f32ne_neon(unsigned n, const float *src, int16_t
> > "movs %[n], %[n], lsr #2 \n\t"
> > "beq 2f \n\t"
> >
> > - "vdup.f32 q2, %[plusone] \n\t"
> > - "vneg.f32 q3, q2 \n\t"
> > - "vdup.f32 q4, %[scale] \n\t"
> > - "vdup.u32 q5, %[mask] \n\t"
> > + "vdup.f32 q1, %[scale] \n\t"
> >
> > "1: \n\t"
> > "vld1.32 {q0}, [%[src]]! \n\t"
> > - "vmin.f32 q0, q0, q2 \n\t" /* clamp */
> > - "vmax.f32 q0, q0, q3 \n\t"
> > - "vmul.f32 q0, q0, q4 \n\t" /* scale */
> > + "vmul.f32 q0, q0, q1 \n\t" /* scale */
> > "vcvt.s32.f32 q0, q0, #16 \n\t" /* narrow */
> You removed clamping - what happens if there's need for clamping? (I'm
> not very good at reading assembly.)
vrshrn does the narrowing int32->int16 (with saturation); the comment
should be moved one line down
> > "vrshrn.s32 d0, q0, #16 \n\t"
> > "subs %[n], %[n], #1 \n\t"
> > @@ -55,13 +50,13 @@ static void pa_sconv_s16le_from_f32ne_neon(unsigned n, const float *src, int16_t
> > "2: \n\t"
> >
> > : [dst] "+r" (dst), [src] "+r" (src), [n] "+r" (n) /* output operands (or input operands that get modified) */
> > - : [plusone] "r" (1.0f), [scale] "r" (32767.0f), [mask] "r" (0x80000000) /* input operands */
> > - : "memory", "cc", "q0", "q1", "q2", "q3", "q4", "q5", "q6" /* clobber list */
> > + : [scale] "r" (32768.0f) /* input operands */
> > + : "memory", "cc", "q0", "q1" /* clobber list */
> > );
> >
> > /* leftovers */
> > while (i--) {
> > - *dst++ = (int16_t) lrintf(PA_CLAMP_UNLIKELY(*src, -1.0f, 1.0f) * 0x7FFF);
> > + *dst++ = (int16_t) PA_CLAMP_UNLIKELY(lrintf(*src * (1 << 15)), -0x8000, 0x7FFF);
> > src++;
> > }
> > }
> > @@ -69,7 +64,7 @@ static void pa_sconv_s16le_from_f32ne_neon(unsigned n, const float *src, int16_t
> > static void pa_sconv_s16le_to_f32ne_neon(unsigned n, const int16_t *src, float *dst) {
> > unsigned i = n & 3;
> >
> > - const float invscale = 1.0f / 0x7FFF;
> > + const float invscale = 1.0f / (1 << 15);
> >
> > __asm__ __volatile__ (
> > "movs %[n], %[n], lsr #2 \n\t"
> > diff --git a/src/pulsecore/sconv_sse.c b/src/pulsecore/sconv_sse.c
> > index 3b56a9c..072d329 100644
> > --- a/src/pulsecore/sconv_sse.c
> > +++ b/src/pulsecore/sconv_sse.c
> > @@ -35,17 +35,13 @@
> >
> > #if !defined(__APPLE__) && defined (__i386__) || defined (__amd64__)
> >
> > -static const PA_DECLARE_ALIGNED (16, float, one[4]) = { 1.0, 1.0, 1.0, 1.0 };
> > -static const PA_DECLARE_ALIGNED (16, float, mone[4]) = { -1.0, -1.0, -1.0, -1.0 };
> > -static const PA_DECLARE_ALIGNED (16, float, scale[4]) = { 0x7fff, 0x7fff, 0x7fff, 0x7fff };
> > +static const PA_DECLARE_ALIGNED (16, float, scale[4]) = { 0x8000, 0x8000, 0x8000, 0x8000 };
> >
> > static void pa_sconv_s16le_from_f32ne_sse(unsigned n, const float *a, int16_t *b) {
> > pa_reg_x86 temp, i;
> >
> > __asm__ __volatile__ (
> > " movaps %5, %%xmm5 \n\t"
> > - " movaps %6, %%xmm6 \n\t"
> > - " movaps %7, %%xmm7 \n\t"
> > " xor %0, %0 \n\t"
> >
> > " mov %4, %1 \n\t"
> > @@ -56,12 +52,8 @@ static void pa_sconv_s16le_from_f32ne_sse(unsigned n, const float *a, int16_t *b
> > "1: \n\t"
> > " movups (%q2, %0, 2), %%xmm0 \n\t" /* read 8 floats */
> > " movups 16(%q2, %0, 2), %%xmm2 \n\t"
> > - " minps %%xmm5, %%xmm0 \n\t" /* clamp to 1.0 */
> > - " minps %%xmm5, %%xmm2 \n\t"
> > - " maxps %%xmm6, %%xmm0 \n\t" /* clamp to -1.0 */
> > - " maxps %%xmm6, %%xmm2 \n\t"
> > - " mulps %%xmm7, %%xmm0 \n\t" /* *= 0x7fff */
> > - " mulps %%xmm7, %%xmm2 \n\t"
> > + " mulps %%xmm5, %%xmm0 \n\t" /* *= 0x8000 */
> > + " mulps %%xmm5, %%xmm2 \n\t"
> >
> > " cvtps2pi %%xmm0, %%mm0 \n\t" /* low part to int */
> > " cvtps2pi %%xmm2, %%mm2 \n\t"
> > @@ -73,7 +65,7 @@ static void pa_sconv_s16le_from_f32ne_sse(unsigned n, const float *a, int16_t *b
> > " packssdw %%mm1, %%mm0 \n\t" /* pack parts */
> > " packssdw %%mm3, %%mm2 \n\t"
> > " movq %%mm0, (%q3, %0) \n\t"
> > - " movq %%mm2, 8(%q3, %0) \n\t"
> > + " movq %%mm2, 8(%q3, %0) \n\t"
> >
> > " add $16, %0 \n\t"
> > " dec %1 \n\t"
> > @@ -82,24 +74,30 @@ static void pa_sconv_s16le_from_f32ne_sse(unsigned n, const float *a, int16_t *b
> > "2: \n\t"
> > " mov %4, %1 \n\t" /* prepare for leftovers */
> > " and $7, %1 \n\t"
> > - " je 4f \n\t"
> > + " je 5f \n\t"
> >
> > "3: \n\t"
> > " movss (%q2, %0, 2), %%xmm0 \n\t"
> > - " minss %%xmm5, %%xmm0 \n\t"
> > - " maxss %%xmm6, %%xmm0 \n\t"
> > - " mulss %%xmm7, %%xmm0 \n\t"
> > + " mulss %%xmm5, %%xmm0 \n\t"
> > + " cvtss2si %%xmm0, %4 \n\t"
> > + " add $0x8000, %4 \n\t" /* check for saturation */
> > + " and $~0xffff, %4 \n\t"
> > " cvtss2si %%xmm0, %4 \n\t"
> > - " movw %w4, (%q3, %0) \n\t"
> > + " je 4f \n\t"
> > + " sar $31, %4 \n\t"
> > + " xor $0x7fff, %4 \n\t"
> > +
> > + "4: \n\t"
> > + " movw %w4, (%q3, %0) \n\t" /* store leftover */
> > " add $2, %0 \n\t"
> > " dec %1 \n\t"
> > " jne 3b \n\t"
> >
> > - "4: \n\t"
> > + "5: \n\t"
> > " emms \n\t"
> >
> > : "=&r" (i), "=&r" (temp)
> > - : "r" (a), "r" (b), "r" ((pa_reg_x86)n), "m" (*one), "m" (*mone), "m" (*scale)
> > + : "r" (a), "r" (b), "r" ((pa_reg_x86)n), "m" (*scale)
> > : "cc", "memory"
> > );
> > }
> > @@ -109,8 +107,6 @@ static void pa_sconv_s16le_from_f32ne_sse2(unsigned n, const float *a, int16_t *
> >
> > __asm__ __volatile__ (
> > " movaps %5, %%xmm5 \n\t"
> > - " movaps %6, %%xmm6 \n\t"
> > - " movaps %7, %%xmm7 \n\t"
> > " xor %0, %0 \n\t"
> >
> > " mov %4, %1 \n\t"
> > @@ -121,12 +117,8 @@ static void pa_sconv_s16le_from_f32ne_sse2(unsigned n, const float *a, int16_t *
> > "1: \n\t"
> > " movups (%q2, %0, 2), %%xmm0 \n\t" /* read 8 floats */
> > " movups 16(%q2, %0, 2), %%xmm2 \n\t"
> > - " minps %%xmm5, %%xmm0 \n\t" /* clamp to 1.0 */
> > - " minps %%xmm5, %%xmm2 \n\t"
> > - " maxps %%xmm6, %%xmm0 \n\t" /* clamp to -1.0 */
> > - " maxps %%xmm6, %%xmm2 \n\t"
> > - " mulps %%xmm7, %%xmm0 \n\t" /* *= 0x7fff */
> > - " mulps %%xmm7, %%xmm2 \n\t"
> > + " mulps %%xmm5, %%xmm0 \n\t" /* *= 0x8000 */
> > + " mulps %%xmm5, %%xmm2 \n\t"
> >
> > " cvtps2dq %%xmm0, %%xmm0 \n\t"
> > " cvtps2dq %%xmm2, %%xmm2 \n\t"
> > @@ -141,23 +133,29 @@ static void pa_sconv_s16le_from_f32ne_sse2(unsigned n, const float *a, int16_t *
> > "2: \n\t"
> > " mov %4, %1 \n\t" /* prepare for leftovers */
> > " and $7, %1 \n\t"
> > - " je 4f \n\t"
> > + " je 5f \n\t"
> >
> > "3: \n\t"
> > " movss (%q2, %0, 2), %%xmm0 \n\t"
> > - " minss %%xmm5, %%xmm0 \n\t"
> > - " maxss %%xmm6, %%xmm0 \n\t"
> > - " mulss %%xmm7, %%xmm0 \n\t"
> > + " mulss %%xmm5, %%xmm0 \n\t"
> > + " cvtss2si %%xmm0, %4 \n\t"
> > + " add $0x8000, %4 \n\t"
> > + " and $~0xffff, %4 \n\t" /* check for saturation */
> > " cvtss2si %%xmm0, %4 \n\t"
> > - " movw %w4, (%q3, %0) \n\t"
> > + " je 4f \n\t"
> > + " sar $31, %4 \n\t"
> > + " xor $0x7fff, %4 \n\t"
> > +
> > + "4: \n\t"
> > + " movw %w4, (%q3, %0) \n\t" /* store leftover */
> > " add $2, %0 \n\t"
> > " dec %1 \n\t"
> > " jne 3b \n\t"
> >
> > - "4: \n\t"
> > + "5: \n\t"
> >
> > : "=&r" (i), "=&r" (temp)
> > - : "r" (a), "r" (b), "r" ((pa_reg_x86)n), "m" (*one), "m" (*mone), "m" (*scale)
> > + : "r" (a), "r" (b), "r" ((pa_reg_x86)n), "m" (*scale)
> > : "cc", "memory"
> > );
> > }
> The x86 code remains unreviewed. I was able to guess to some extent
> what's happening in the arm code, but can't do that for the x86 code.
> Maybe Arun will check this, or maybe I should learn this stuff properly.
packssdw does pack with signed saturation
regards, p.
--
Peter Meerwald
+43-664-2444418 (mobile)
