skia: Simplify SSE2 logic a bit.

Chrome builds with -msse2 globally these days, so remove the skia_chrome_opts target and fold it into skia_chrome. Keep skia_opts but simplify the comments there a bit. No intended behavior change. BUG=496512,348761 Review URL: https://codereview.chromium.org/1161853008 Cr-Commit-Position: refs/heads/master@{#333129}
marcosholgado · Jun 5, 2015 · 1281ac7 · 1281ac7
1 parent ed24c2c
commit 1281ac7
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 71 deletions.
diff --git a/skia/BUILD.gn b/skia/BUILD.gn
@@ -318,6 +318,18 @@ component("skia") {
     "ext/skia_utils_win.h",
   ]
 
+  if (current_cpu == "x86" || current_cpu == "x64") {
+    sources += [
+      "ext/convolver_SSE2.cc",
+      "ext/convolver_SSE2.h",
+    ]
+  } else if (current_cpu == "mipsel" && mips_dsp_rev >= 2) {
+    sources += [
+      "ext/convolver_mips_dspr2.cc",
+      "ext/convolver_mips_dspr2.h",
+    ]
+  }
+
   # The skia gypi values are relative to the skia_dir, so we need to rebase.
   sources += gypi_skia_core.sources
   sources += gypi_skia_effects.sources
@@ -552,12 +564,7 @@ source_set("skia_opts") {
 
   if (current_cpu == "x86" || current_cpu == "x64") {
     sources = gypi_skia_opts.sse2_sources + gypi_skia_opts.ssse3_sources +
-              gypi_skia_opts.sse41_sources +
-              [
-                # Chrome-specific.
-                "ext/convolver_SSE2.cc",
-                "ext/convolver_SSE2.h",
-              ]
+              gypi_skia_opts.sse41_sources
 
     if (!is_win || is_clang) {
       cflags += [ "-msse4.1" ]
@@ -590,13 +597,6 @@ source_set("skia_opts") {
 
     if (mips_dsp_rev >= 1) {
       sources = gypi_skia_opts.mips_dsp_sources
-      if (mips_dsp_rev >= 2) {
-        sources += [
-          # Chrome-specific.
-          "ext/convolver_mips_dspr2.cc",
-          "ext/convolver_mips_dspr2.h",
-        ]
-      }
     } else {
       sources = gypi_skia_opts.none_sources
     }

diff --git a/skia/skia.gyp b/skia/skia.gyp
@@ -87,37 +87,6 @@
 
   # targets that are not dependent upon the component type
   'targets': [
-    {
-      'target_name': 'skia_chrome_opts',
-      'type': 'static_library',
-      'include_dirs': [
-        '..',
-        'config',
-        '../third_party/skia/include/core',
-      ],
-      'conditions': [
-        [ 'os_posix == 1 and OS != "mac" and OS != "android" and \
-            target_arch != "arm" and target_arch != "mipsel" and \
-            target_arch != "arm64" and target_arch != "mips64el"', {
-          'cflags': [
-            '-msse2',
-          ],
-        }],
-        [ 'target_arch != "arm" and target_arch != "mipsel" and \
-           target_arch != "arm64" and target_arch != "mips64el"', {
-          'sources': [
-            'ext/convolver_SSE2.cc',
-            'ext/convolver_SSE2.h',
-          ],
-        }],
-        [ 'target_arch == "mipsel" and mips_dsp_rev >= 2',{
-          'sources': [
-            'ext/convolver_mips_dspr2.cc',
-            'ext/convolver_mips_dspr2.h',
-          ],
-        }],
-      ],
-    },
     {
       'target_name': 'image_operations_bench',
       'type': 'executable',

diff --git a/skia/skia_chrome.gypi b/skia/skia_chrome.gypi
@@ -9,7 +9,6 @@
 {
   'dependencies': [
     'skia_library',
-    'skia_chrome_opts',
     '../base/base.gyp:base',
     '../base/third_party/dynamic_annotations/dynamic_annotations.gyp:dynamic_annotations',
   ],
@@ -85,16 +84,24 @@
         'ext/skia_utils_base.cc',
       ],
     }],
-    ['OS == "ios"', {
-      'dependencies!': [
-        'skia_chrome_opts',
-      ],
-    }],
     [ 'OS != "android" and (OS != "linux" or use_cairo==1)', {
       'sources!': [
         'ext/bitmap_platform_device_skia.cc',
       ],
     }],
+    [ 'OS != "ios" and target_arch != "arm" and target_arch != "mipsel" and \
+       target_arch != "arm64" and target_arch != "mips64el"', {
+      'sources': [
+        'ext/convolver_SSE2.cc',
+        'ext/convolver_SSE2.h',
+      ],
+    }],
+    [ 'target_arch == "mipsel" and mips_dsp_rev >= 2',{
+      'sources': [
+        'ext/convolver_mips_dspr2.cc',
+        'ext/convolver_mips_dspr2.h',
+      ],
+    }],
   ],
 
   'target_conditions': [

diff --git a/skia/skia_library_opts.gyp b/skia/skia_library_opts.gyp
@@ -18,22 +18,10 @@
    },
 
   'targets': [
-    # Due to an unfortunate intersection of lameness between gcc and gyp,
-    # we have to build the *_SSE2.cpp files in a separate target.  The
-    # gcc lameness is that, in order to compile SSE2 intrinsics code, it
-    # must be passed the -msse2 flag.  However, with this flag, it may
-    # emit SSE2 instructions even for scalar code, such as the CPUID
-    # test used to test for the presence of SSE2.  So that, and all other
-    # code must be compiled *without* -msse2.  The gyp lameness is that it
-    # does not allow file-specific CFLAGS, so we must create this extra
-    # target for those files to be compiled with -msse2.
-    #
-    # This is actually only a problem on 32-bit Linux (all Intel Macs have
-    # SSE2, Linux x86_64 has SSE2 by definition, and MSC will happily emit
-    # SSE2 from instrinsics, which generating plain ol' 386 for everything
-    # else).  However, to keep the .gyp file simple and avoid platform-specific
-    # build breakage, we do this on all platforms.
-
+    # SSE files have to be built in a separate target, because gcc needs
+    # different -msse flags for different SSE levels which enable use of SSE
+    # intrinsics but also allow emission of SSE2 instructions for scalar code.
+    # gyp does not allow per-file compiler flags.
     # For about the same reason, we need to compile the ARM opts files
     # separately as well.
     {
@@ -49,13 +37,12 @@
       ],
       'include_dirs': [ '<@(include_dirs)' ],
       'conditions': [
-        [ 'os_posix == 1 and OS != "mac" and OS != "android" and \
-           target_arch != "arm" and target_arch != "arm64" and \
-           target_arch != "mipsel" and target_arch != "mips64el"', {
-          'cflags': [ '-msse2' ],
-        }],
         [ 'target_arch != "arm" and target_arch != "mipsel" and \
            target_arch != "arm64" and target_arch != "mips64el"', {
+          # Chrome builds with -msse2 locally, so sse2_sources could in theory
+          # be in the regular skia target. But we need skia_opts for arm
+          # anyway, so putting sse2_sources here is simpler than making this
+          # conditionally a type none target on x86.
           'sources': [ '<@(sse2_sources)' ],
           'dependencies': [
             'skia_opts_ssse3',