SkOpts_hsw ODR paranoia
authorMike Klein <mtklein@chromium.org>
Thu, 26 Jan 2017 16:41:03 +0000 (11:41 -0500)
committerSkia Commit-Bot <skia-commit-bot@chromium.org>
Thu, 26 Jan 2017 18:22:38 +0000 (18:22 +0000)
I'm warming back up to the idea of very careful use of SkOpts_hsw.
But if we're going to do that, we need a strict header discipline.
No header can be assumed to be safe without vetting, and most aren't.

Today there's only one function defined in SkOpts_hsw, so this CL mostly
rewrites that convolve_vertically() to use no headers beyond immintrin.h
and stdint.h, both safe.  It shared very little code with the others
anyway, so we're not losing anything by putting it directly into
SkOpts_hsw.cpp.  I have also streamlined the implementation considerably
to improve maintainability and readability.

Change-Id: Ia03daae660e54125a0d2e2988464cfc930349e80
Reviewed-on: https://skia-review.googlesource.com/7611
Reviewed-by: Matt Sarett <msarett@google.com>
Commit-Queue: Mike Klein <mtklein@chromium.org>

src/opts/SkBitmapFilter_opts.h
src/opts/SkOpts_hsw.cpp

index b6199cb..4f21c57 100644 (file)
 
 namespace SK_OPTS_NS {
 
-#if SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_AVX2
-
-    static SK_ALWAYS_INLINE
-    void compute_coefficient_row(SkConvolutionFilter1D::ConvolutionFixed filterValue, const unsigned char* sourceDataRows,
-                               __m256i* accum01, __m256i* accum23, __m256i* accum45, __m256i* accum67) {
-        __m256i coefs = _mm256_set1_epi16(filterValue);
-        __m256i pixels = _mm256_loadu_si256(reinterpret_cast<const __m256i *>(sourceDataRows));
-        __m256i zero = _mm256_setzero_si256();
-
-        // [16] a3 b3 g3 r3 a2 b2 g2 r2 a1 b1 g1 r1 a0 b0 g0 r0
-        __m256i pixels_0123_16bit = _mm256_unpacklo_epi8(pixels, zero);
-
-        __m256i scaled_0123_hi = _mm256_mulhi_epi16(pixels_0123_16bit, coefs),
-                scaled_0123_lo = _mm256_mullo_epi16(pixels_0123_16bit, coefs);
-
-        // [32] c*a1 c*b1 c*g1 c*r1 c*a0 c*b0 c*g0 c*r0
-        *accum01 = _mm256_add_epi32(*accum01, _mm256_unpacklo_epi16(scaled_0123_lo, scaled_0123_hi));
-        // [32] c*a3 c*b3 c*g3 c*r3 c*a2 c*b2 c*g2 c*r2
-        *accum23 = _mm256_add_epi32(*accum23, _mm256_unpackhi_epi16(scaled_0123_lo, scaled_0123_hi));
-
-        // [16] a7 b7 g7 r7 a6 b6 g6 r6 a5 b5 g5 r5 a4 b4 g4 r4
-        __m256i pixels_4567_16bit = _mm256_unpackhi_epi8(pixels, zero);
-
-        __m256i scaled_4567_hi = _mm256_mulhi_epi16(pixels_4567_16bit, coefs),
-                scaled_4567_lo = _mm256_mullo_epi16(pixels_4567_16bit, coefs);
-
-        // [32] c*a5 c*b5 c*g5 c*r5 c*a4 c*b4 c*g4 c*r4
-        *accum45 = _mm256_add_epi32(*accum45, _mm256_unpacklo_epi16(scaled_4567_lo, scaled_4567_hi));
-        // [32] c*a7 c*b7 c*g7 c*r7 c*a6 c*b6 c*g6 c*r6
-        *accum67 = _mm256_add_epi32(*accum67, _mm256_unpackhi_epi16(scaled_4567_lo, scaled_4567_hi));
-    }
-
-    template<bool hasAlpha>
-    void ConvolveVertically(const SkConvolutionFilter1D::ConvolutionFixed* filterValues,
-                            int filterLength,
-                            unsigned char* const * sourceDataRows,
-                            int pixelWidth,
-                            unsigned char* outRow) {
-        // Output eight pixels per iteration (32 bytes).
-        for (int outX = 0; outX < pixelWidth; outX += 8) {
-            // Accumulated result for each pixel. 32 bits per RGBA channel.
-            __m256i accum01 = _mm256_setzero_si256();
-            __m256i accum23 = _mm256_setzero_si256();
-            __m256i accum45 = _mm256_setzero_si256();
-            __m256i accum67 = _mm256_setzero_si256();
-
-            // Convolve with 4 filter coefficient per iteration.
-            int length = filterLength & ~3;
-            for (int filterY = 0; filterY < length; filterY += 4) {
-                compute_coefficient_row(filterValues[filterY + 0], sourceDataRows[filterY + 0] + outX * 4, &accum01, &accum23, &accum45, &accum67);
-                compute_coefficient_row(filterValues[filterY + 1], sourceDataRows[filterY + 1] + outX * 4, &accum01, &accum23, &accum45, &accum67);
-                compute_coefficient_row(filterValues[filterY + 2], sourceDataRows[filterY + 2] + outX * 4, &accum01, &accum23, &accum45, &accum67);
-                compute_coefficient_row(filterValues[filterY + 3], sourceDataRows[filterY + 3] + outX * 4, &accum01, &accum23, &accum45, &accum67);
-            }
-            for (int filterY = length; filterY < filterLength; filterY++) {
-                compute_coefficient_row(filterValues[filterY], sourceDataRows[filterY] + outX * 4, &accum01, &accum23, &accum45, &accum67);
-            }
-
-            // Shift right for fixed point implementation.
-            accum01 = _mm256_srai_epi32(accum01, SkConvolutionFilter1D::kShiftBits);
-            accum23 = _mm256_srai_epi32(accum23, SkConvolutionFilter1D::kShiftBits);
-            accum45 = _mm256_srai_epi32(accum45, SkConvolutionFilter1D::kShiftBits);
-            accum67 = _mm256_srai_epi32(accum67, SkConvolutionFilter1D::kShiftBits);
-
-            // Packing 32 bits |accum| to 16 bits per channel (signed saturation).
-            // [16] a3 b3 g3 r3 a2 b2 g2 r2 a1 b1 g1 r1 a0 b0 g0 r0
-            __m256i accum_0123 = _mm256_packs_epi32(accum01, accum23);
-
-            // Packing 32 bits |accum| to 16 bits per channel (signed saturation).
-            // [16] a7 b7 g7 r7 a6 b6 g6 r6 a5 b5 g5 r5 a4 b4 g4 r4
-            __m256i accum_4567 = _mm256_packs_epi32(accum45, accum67);
-
-            // Packing 16 bits |accum| to 8 bits per channel (unsigned saturation).
-            //  [8] a7 b7 g7 r7 a6 b6 g6 r6 a5 b5 g5 r5 a4 b4 g4 r4 a3 b3 g3 r3 a2 b2 g2 r2 a1 b1 g1 r1 a0 b0 g0 r0
-            __m256i accum = _mm256_packus_epi16(accum_0123, accum_4567);
-
-            if (hasAlpha) {
-                // Make sure the value of alpha channel is always larger than maximum
-                // value of color channels.
-                // If alpha is less than r, g, or b, set it to their max.
-                __m256i max_rg  = _mm256_max_epu8( accum, _mm256_srli_epi32(accum,  8));
-                __m256i max_rgb = _mm256_max_epu8(max_rg, _mm256_srli_epi32(accum, 16));
-                accum = _mm256_max_epu8(accum, _mm256_slli_epi32(max_rgb, 24));
-            } else {
-                // Force opaque.
-                accum = _mm256_or_si256(accum, _mm256_set1_epi32(0xff000000));
-            }
-
-            // Store the convolution result (32 bytes) and advance the pixel pointers.
-            // During the last iteration, when pixels left are less than 8, store them one at a time.
-            if (outX + 8 <= pixelWidth) {
-                _mm256_storeu_si256(reinterpret_cast<__m256i *>(outRow), accum);
-                outRow += 32;
-            } else {
-                for (int i = outX; i < pixelWidth; i++) {
-                    *(reinterpret_cast<int*>(outRow)) = _mm_cvtsi128_si32(_mm256_castsi256_si128(accum));
-                    __m256i rotate = _mm256_setr_epi32(1, 2, 3, 4, 5, 6, 7, 0);
-                    accum = _mm256_permutevar8x32_epi32(accum, rotate);
-                    outRow += 4;
-                }
-            }
-        }
-    }
-#endif
-
 #if SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSE2
 
     static SK_ALWAYS_INLINE void AccumRemainder(const unsigned char* pixelsLeft,
@@ -336,8 +231,6 @@ namespace SK_OPTS_NS {
         }
     }
 
-// If we've got AVX2, we've already defined a faster ConvolveVertically above.
-#if SK_CPU_SSE_LEVEL < SK_CPU_SSE_LEVEL_AVX2
     // Does vertical convolution to produce one output row. The filter values and
     // length are given in the first two parameters. These are applied to each
     // of the rows pointed to in the |sourceDataRows| array, with each row
@@ -508,7 +401,6 @@ namespace SK_OPTS_NS {
             }
         }
     }
-#endif//SK_CPU_SSE_LEVEL < SK_CPU_SSE_LEVEL_AVX2
 
 #elif defined(SK_ARM_HAS_NEON)
 
index 223495f..def1b3c 100644 (file)
@@ -5,25 +5,91 @@
  * found in the LICENSE file.
  */
 
-#include "SkSafe_math.h"   // Keep this first.
+// It is not safe to #include any header file here unless it has been vetted for ODR safety:
+// all symbols used must be file-scoped static or in an anonymous namespace.  This applies
+// to _all_ header files:  C standard library, C++ standard library, Skia... everything.
 
-// Please note carefully.
-// It is not safe for _opts.h files included here to use STL types, for the
-// same reason we just had to include SkSafe_math.h: STL types are templated,
-// defined in headers, but not in anonymous namespaces.  It's very easy to
-// cause ODR violations with these types and AVX+ code generation.
+#include <immintrin.h>   // ODR safe
+#include <stdint.h>      // ODR safe
 
-#include "SkOpts.h"
-#define SK_OPTS_NS hsw
-#include "SkBitmapFilter_opts.h"
+namespace hsw {
 
-#if defined(_INC_MATH) && !defined(INC_MATH_IS_SAFE_NOW)
-    #error We have included ucrt\math.h without protecting it against ODR violation.
-#endif
+    void convolve_vertically(const int16_t* filter, int filterLen,
+                             uint8_t* const* srcRows, int width,
+                             uint8_t* out, bool hasAlpha) {
+        // It's simpler to work with the output array in terms of 4-byte pixels.
+        auto dst = (int*)out;
+
+        // Output up to eight pixels per iteration.
+        for (int x = 0; x < width; x += 8) {
+            // Accumulated result for 4 adjacent pairs of pixels, in signed 17.14 fixed point.
+            auto accum01 = _mm256_setzero_si256(),
+                 accum23 = _mm256_setzero_si256(),
+                 accum45 = _mm256_setzero_si256(),
+                 accum67 = _mm256_setzero_si256();
+
+            // Convolve with the filter.  (This inner loop is where we spend ~all our time.)
+            for (int i = 0; i < filterLen; i++) {
+                auto coeffs = _mm256_set1_epi16(filter[i]);
+                auto pixels = _mm256_loadu_si256((const __m256i*)(srcRows[i] + x*4));
+
+                auto pixels_0123 = _mm256_unpacklo_epi8(pixels, _mm256_setzero_si256()),
+                     pixels_4567 = _mm256_unpackhi_epi8(pixels, _mm256_setzero_si256());
+
+                auto lo_0123 = _mm256_mullo_epi16(pixels_0123, coeffs),
+                     hi_0123 = _mm256_mulhi_epi16(pixels_0123, coeffs),
+                     lo_4567 = _mm256_mullo_epi16(pixels_4567, coeffs),
+                     hi_4567 = _mm256_mulhi_epi16(pixels_4567, coeffs);
+
+                accum01 = _mm256_add_epi32(accum01, _mm256_unpacklo_epi16(lo_0123, hi_0123));
+                accum23 = _mm256_add_epi32(accum23, _mm256_unpackhi_epi16(lo_0123, hi_0123));
+                accum45 = _mm256_add_epi32(accum45, _mm256_unpacklo_epi16(lo_4567, hi_4567));
+                accum67 = _mm256_add_epi32(accum67, _mm256_unpackhi_epi16(lo_4567, hi_4567));
+            }
+
+            // Trim the fractional parts.
+            accum01 = _mm256_srai_epi32(accum01, 14);
+            accum23 = _mm256_srai_epi32(accum23, 14);
+            accum45 = _mm256_srai_epi32(accum45, 14);
+            accum67 = _mm256_srai_epi32(accum67, 14);
+
+            // Pack back down to 8-bit channels.
+            auto pixels = _mm256_packus_epi16(_mm256_packs_epi32(accum01, accum23),
+                                              _mm256_packs_epi32(accum45, accum67));
+
+            if (hasAlpha) {
+                // Clamp alpha to the max of r,g,b to make sure we stay premultiplied.
+                __m256i max_rg  = _mm256_max_epu8(pixels, _mm256_srli_epi32(pixels,  8)),
+                        max_rgb = _mm256_max_epu8(max_rg, _mm256_srli_epi32(pixels, 16));
+                pixels = _mm256_max_epu8(pixels, _mm256_slli_epi32(max_rgb, 24));
+            } else {
+                // Force opaque.
+                pixels = _mm256_or_si256(pixels, _mm256_set1_epi32(0xff000000));
+            }
+
+            // Normal path to store 8 pixels.
+            if (x + 8 <= width) {
+                _mm256_storeu_si256((__m256i*)dst, pixels);
+                dst += 8;
+                continue;
+            }
+
+            // Store one pixel at a time on the last iteration.
+            for (int i = x; i < width; i++) {
+                *dst++ = _mm_cvtsi128_si32(_mm256_castsi256_si128(pixels));
+                pixels = _mm256_permutevar8x32_epi32(pixels, _mm256_setr_epi32(1,2,3,4,5,6,7,0));
+            }
+        }
+    }
+
+}
 
 namespace SkOpts {
+    // See SkOpts.h, writing SkConvolutionFilter1D::ConvolutionFixed as the underlying type.
+    extern void (*convolve_vertically)(const int16_t* filter, int filterLen,
+                                       uint8_t* const* srcRows, int width,
+                                       uint8_t* out, bool hasAlpha);
     void Init_hsw() {
         convolve_vertically = hsw::convolve_vertically;
     }
 }
-