Make SSE2/Neon convolution functions not to read extra bytes

src/opts/SkBitmapFilter_opts_SSE2.cpp

 /*
  * Copyright 2013 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include <emmintrin.h>
 #include "SkBitmap.h"
 #include "SkBitmapFilter_opts_SSE2.h"
@@ skipping 32 matching lines @@
 // Convolves horizontally along a single row. The row data is given in
 // |src_data| and continues for the num_values() of the filter.
 void convolveHorizontally_SSE2(const unsigned char* src_data,
                                const SkConvolutionFilter1D& filter,
                                unsigned char* out_row,
                                bool /*has_alpha*/) {
     int num_values = filter.numValues();
 
     int filter_offset, filter_length;
     __m128i zero = _mm_setzero_si128();
-    __m128i mask[4];
-    // |mask| will be used to decimate all extra filter coefficients that are
-    // loaded by SIMD when |filter_length| is not divisible by 4.
-    // mask[0] is not used in following algorithm.
-    mask[1] = _mm_set_epi16(0, 0, 0, 0, 0, 0, 0, -1);
-    mask[2] = _mm_set_epi16(0, 0, 0, 0, 0, 0, -1, -1);
-    mask[3] = _mm_set_epi16(0, 0, 0, 0, 0, -1, -1, -1);
 
     // Output one pixel each iteration, calculating all channels (RGBA) together.
     for (int out_x = 0; out_x < num_values; out_x++) {
         const SkConvolutionFilter1D::ConvolutionFixed* filter_values =
             filter.FilterForValue(out_x, &filter_offset, &filter_length);
 
         __m128i accum = _mm_setzero_si128();
 
         // Compute the first pixel in this row that the filter affects. It will
         // touch |filter_length| pixels (4 bytes each) after this.
@@ skipping 43 matching lines @@
             accum = _mm_add_epi32(accum, t);
             // [32]  a3*c3 b3*c3 g3*c3 r3*c3
             t = _mm_unpackhi_epi16(mul_lo, mul_hi);
             accum = _mm_add_epi32(accum, t);
 
             // Advance the pixel and coefficients pointers.
             row_to_filter += 1;
             filter_values += 4;
         }
 
-        // When |filter_length| is not divisible by 4, we need to decimate some of
-        // the filter coefficient that was loaded incorrectly to zero; Other than
-        // that the algorithm is same with above, exceot that the 4th pixel will be
-        // always absent.
-        int r = filter_length&3;
+        // When |filter_length| is not divisible by 4, we accumulate the last 1 - 3
+        // coefficients one at a time.
+        int r = filter_length & 3;
         if (r) {
-            // Note: filter_values must be padded to align_up(filter_offset, 8).
-            __m128i coeff, coeff16;
-            coeff = _mm_loadl_epi64(reinterpret_cast<const __m128i*>(filter_values));
-            // Mask out extra filter taps.
-            coeff = _mm_and_si128(coeff, mask[r]);
-            coeff16 = _mm_shufflelo_epi16(coeff, _MM_SHUFFLE(1, 1, 0, 0));
-            coeff16 = _mm_unpacklo_epi16(coeff16, coeff16);
-
-            // Note: line buffer must be padded to align_up(filter_offset, 16).
-            // We resolve this by use C-version for the last horizontal line.
-            __m128i src8 = _mm_loadu_si128(row_to_filter);
-            __m128i src16 = _mm_unpacklo_epi8(src8, zero);
-            __m128i mul_hi = _mm_mulhi_epi16(src16, coeff16);
-            __m128i mul_lo = _mm_mullo_epi16(src16, coeff16);
-            __m128i t = _mm_unpacklo_epi16(mul_lo, mul_hi);
-            accum = _mm_add_epi32(accum, t);
-            t = _mm_unpackhi_epi16(mul_lo, mul_hi);
-            accum = _mm_add_epi32(accum, t);
-
-            src16 = _mm_unpackhi_epi8(src8, zero);
-            coeff16 = _mm_shufflelo_epi16(coeff, _MM_SHUFFLE(3, 3, 2, 2));
-            coeff16 = _mm_unpacklo_epi16(coeff16, coeff16);
-            mul_hi = _mm_mulhi_epi16(src16, coeff16);
-            mul_lo = _mm_mullo_epi16(src16, coeff16);
-            t = _mm_unpacklo_epi16(mul_lo, mul_hi);
-            accum = _mm_add_epi32(accum, t);
+#define ACCUM_REMAINDER(src, accum) {                                                          \

[mtklein_C, 2016/11/09 09:25:04]

Can we make these ACCUM_REMAINDERs lambdas or static functions instead of
macros?  I don't see anything too sneaky going on here that really requires a
macro.  If needed, SK_ALWAYS_INLINE is pretty reliable at forcing inlining.

(Same question in the NEON code of course.)

[xiangze.zhang, 2016/11/10 03:16:56]

Done.

+            int remainder[4] = {0};                                                            \
+            const unsigned char* pixels_left = src + (filter_offset + filter_length - r) * 4;  \
+            for (int i = 0; i < r; i++) {                                                      \
+                SkConvolutionFilter1D::ConvolutionFixed coeff = filter_values[i];              \
+                remainder[0] += coeff * pixels_left[i * 4 + 0];                                \
+                remainder[1] += coeff * pixels_left[i * 4 + 1];                                \
+                remainder[2] += coeff * pixels_left[i * 4 + 2];                                \
+                remainder[3] += coeff * pixels_left[i * 4 + 3];                                \
+            }                                                                                  \
+            __m128i t = _mm_set_epi32(remainder[3], remainder[2], remainder[1], remainder[0]); \

[mtklein_C, 2016/11/09 09:25:04]

This is not a big deal, but we have been trending towards using _mm_setr
versions of these functions, mostly to keep things reading consistently when you
switch over to NEON or think about how the vector would be stored in memory.

[xiangze.zhang, 2016/11/10 03:16:56]

Done.

+            accum = _mm_add_epi32(accum, t); }

[mtklein_C, 2016/11/09 09:25:04]

If we do need to use macros, it's probably best to add another \ and put the }
on its own line.  It's sort of easy to overlook here.

+            ACCUM_REMAINDER(src_data, accum);
         }
 
         // Shift right for fixed point implementation.
         accum = _mm_srai_epi32(accum, SkConvolutionFilter1D::kShiftBits);
 
         // Packing 32 bits |accum| to 16 bits per channel (signed saturation).
         accum = _mm_packs_epi32(accum, zero);
         // Packing 16 bits |accum| to 8 bits per channel (unsigned saturation).
         accum = _mm_packus_epi16(accum, zero);
 
@@ skipping 10 matching lines @@
 void convolve4RowsHorizontally_SSE2(const unsigned char* src_data[4],
                                     const SkConvolutionFilter1D& filter,
                                     unsigned char* out_row[4],
                                     size_t outRowBytes) {
     SkDEBUGCODE(const unsigned char* out_row_0_start = out_row[0];)
 
     int num_values = filter.numValues();
 
     int filter_offset, filter_length;
     __m128i zero = _mm_setzero_si128();
-    __m128i mask[4];
-    // |mask| will be used to decimate all extra filter coefficients that are
-    // loaded by SIMD when |filter_length| is not divisible by 4.
-    // mask[0] is not used in following algorithm.
-    mask[1] = _mm_set_epi16(0, 0, 0, 0, 0, 0, 0, -1);
-    mask[2] = _mm_set_epi16(0, 0, 0, 0, 0, 0, -1, -1);
-    mask[3] = _mm_set_epi16(0, 0, 0, 0, 0, -1, -1, -1);
 
     // Output one pixel each iteration, calculating all channels (RGBA) together.
     for (int out_x = 0; out_x < num_values; out_x++) {
         const SkConvolutionFilter1D::ConvolutionFixed* filter_values =
             filter.FilterForValue(out_x, &filter_offset, &filter_length);
 
         // four pixels in a column per iteration.
         __m128i accum0 = _mm_setzero_si128();
         __m128i accum1 = _mm_setzero_si128();
         __m128i accum2 = _mm_setzero_si128();
@@ skipping 36 matching lines @@
             ITERATION(src_data[1] + start, accum1);
             ITERATION(src_data[2] + start, accum2);
             ITERATION(src_data[3] + start, accum3);
 
             start += 16;
             filter_values += 4;
         }
 
         int r = filter_length & 3;
         if (r) {
-            // Note: filter_values must be padded to align_up(filter_offset, 8);
-            __m128i coeff;
-            coeff = _mm_loadl_epi64(reinterpret_cast<const __m128i*>(filter_values));
-            // Mask out extra filter taps.
-            coeff = _mm_and_si128(coeff, mask[r]);
-
-            __m128i coeff16lo = _mm_shufflelo_epi16(coeff, _MM_SHUFFLE(1, 1, 0, 0));
-            /* c1 c1 c1 c1 c0 c0 c0 c0 */
-            coeff16lo = _mm_unpacklo_epi16(coeff16lo, coeff16lo);
-            __m128i coeff16hi = _mm_shufflelo_epi16(coeff, _MM_SHUFFLE(3, 3, 2, 2));
-            coeff16hi = _mm_unpacklo_epi16(coeff16hi, coeff16hi);
-
-            __m128i src8, src16, mul_hi, mul_lo, t;
-
-            ITERATION(src_data[0] + start, accum0);
-            ITERATION(src_data[1] + start, accum1);
-            ITERATION(src_data[2] + start, accum2);
-            ITERATION(src_data[3] + start, accum3);
+            ACCUM_REMAINDER(src_data[0], accum0);
+            ACCUM_REMAINDER(src_data[1], accum1);
+            ACCUM_REMAINDER(src_data[2], accum2);
+            ACCUM_REMAINDER(src_data[3], accum3);
         }
 
         accum0 = _mm_srai_epi32(accum0, SkConvolutionFilter1D::kShiftBits);
         accum0 = _mm_packs_epi32(accum0, zero);
         accum0 = _mm_packus_epi16(accum0, zero);
         accum1 = _mm_srai_epi32(accum1, SkConvolutionFilter1D::kShiftBits);
         accum1 = _mm_packs_epi32(accum1, zero);
         accum1 = _mm_packus_epi16(accum1, zero);
         accum2 = _mm_srai_epi32(accum2, SkConvolutionFilter1D::kShiftBits);
         accum2 = _mm_packs_epi32(accum2, zero);
@@ skipping 204 matching lines @@
                                       pixel_width,
                                       out_row);
     } else {
         convolveVertically_SSE2<false>(filter_values,
                                        filter_length,
                                        source_data_rows,
                                        pixel_width,
                                        out_row);
     }
 }
-
-void applySIMDPadding_SSE2(SkConvolutionFilter1D *filter) {
-    // Padding |paddingCount| of more dummy coefficients after the coefficients
-    // of last filter to prevent SIMD instructions which load 8 or 16 bytes
-    // together to access invalid memory areas. We are not trying to align the
-    // coefficients right now due to the opaqueness of <vector> implementation.
-    // This has to be done after all |AddFilter| calls.
-    for (int i = 0; i < 8; ++i) {
-        filter->addFilterValue(static_cast<SkConvolutionFilter1D::ConvolutionFixed>(0));
-    }
-}