Skip to content

Commit

Permalink
add compile-time not-supported checks to runtime CPU checks
Browse files Browse the repository at this point in the history
There's no reason to call Init_ssse3() if we know Skia's built globally
with SSSE3.  And there's definitely no reason to call Init_ssse3() if
Skia's build globally with SSE4.1+.

These are the only places the Init_foo() methods are called, and those
in turn are the only places that refer to their optimized routines, so
guarding like this allows redundant routines to be dead-code stripped by
the linker.

There are still situations where we end up with two copies of the same
routine, compiled at _the same_ optimization level.  If you're on a Mac
and have SSSE3 or SSE4.1 as your global baseline instruction set, you'll
get two copies, one from SkOpts.o's defaults, and one from
SkOpts_{ssse3,sse41}.o's "better" routines.  I'm still thinking about
how to best fix this.  Might just be as simple as removing "static" and
letting the linker dedup.

This cuts off about 70K of code on a build of ok.

Change-Id: Ia349d2c5299072bbd43966132798500de059b9ca
Reviewed-on: https://skia-review.googlesource.com/37600
Reviewed-by: Mike Reed <reed@google.com>
Commit-Queue: Mike Klein <mtklein@chromium.org>
  • Loading branch information
Mike Klein authored and Skia Commit-Bot committed Aug 23, 2017
1 parent 5127998 commit bb29f43
Showing 1 changed file with 15 additions and 4 deletions.
19 changes: 15 additions & 4 deletions src/core/SkOpts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,21 @@ namespace SkOpts {
static void init() {
#if !defined(SK_BUILD_NO_OPTS)
#if defined(SK_CPU_X86)
if (SkCpu::Supports(SkCpu::SSSE3)) { Init_ssse3(); }
if (SkCpu::Supports(SkCpu::SSE41)) { Init_sse41(); }
if (SkCpu::Supports(SkCpu::SSE42)) { Init_sse42(); }
if (SkCpu::Supports(SkCpu::AVX )) { Init_avx(); }
#if SK_CPU_SSE_LEVEL < SK_CPU_SSE_LEVEL_SSSE3
if (SkCpu::Supports(SkCpu::SSSE3)) { Init_ssse3(); }
#endif

#if SK_CPU_SSE_LEVEL < SK_CPU_SSE_LEVEL_SSE41
if (SkCpu::Supports(SkCpu::SSE41)) { Init_sse41(); }
#endif

#if SK_CPU_SSE_LEVEL < SK_CPU_SSE_LEVEL_SSE42
if (SkCpu::Supports(SkCpu::SSE42)) { Init_sse42(); }
#endif

#if SK_CPU_SSE_LEVEL < SK_CPU_SSE_LEVEL_AVX
if (SkCpu::Supports(SkCpu::AVX )) { Init_avx(); }
#endif

#elif defined(SK_CPU_ARM64)
if (SkCpu::Supports(SkCpu::CRC32)) { Init_crc32(); }
Expand Down

2 comments on commit bb29f43

@mikekaganski
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this means that the Init_* must be compiled and available to linker; and in turn, it means that the compilation units with these functions (each of those built with own instruction set support) is passed to linker; given how much stuff is indirectly included into e.g. src/core/SkOpts.h (included, say, into src/opts/SkOpts_avx.cpp) - e.g., src/core/SkRasterPipeline.h -> include/core/SkMatrix.h -> include/core/SkRect.h, there will be several implementations of e.g. SkRect::round available for the linker, and it will be free to choose whichever it likes, including the one using greater instruction set, crashing an app using this function on a CPU with a lower instruction set. See e.g. https://bugs.documentfoundation.org/show_bug.cgi?id=144598, with comment 15 providing two disassemblies of non-ilined SkRect::round from different packages, both compiled with SSE2 level.

@mtklein
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh boy yeah, this is classic ODR violation. Our typical approach to avoid this was to mark any function defined in a header as either static or force-inlined, but looks like that's not true at all for include/core/SkRect.h.

Please sign in to comment.