Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
272503006:
Improved x86 SSE build and run-time checks. (Closed)
|
Created:
6 years, 7 months ago by henrik.smiding Modified:
6 years, 7 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlesource.com/skia.git@master Visibility:
Public. |
DescriptionImproved x86 SSE build and run-time checks.
Replaces the current build/run-time checks for SSE level in
opts_check_x86.cpp with a simpler and more future-proof version.
Also adds SSE versions 4.1 and 4.2 to the config file.
Author: henrik.smiding@intel.com
Signed-off-by: Henrik Smiding <henrik.smiding@intel.com>;
Committed: http://code.google.com/p/skia/source/detail?r=14644
Committed: http://code.google.com/p/skia/source/detail?r=14693
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fixed review comments #Patch Set 3 : Check for undefined SSE flag #
Total comments: 2
Patch Set 4 : Added 64-bit check for VisualStudio #Messages
Total messages: 20 (0 generated)
https://codereview.chromium.org/272503006/diff/1/include/core/SkPreConfig.h File include/core/SkPreConfig.h (right): https://codereview.chromium.org/272503006/diff/1/include/core/SkPreConfig.h#n... include/core/SkPreConfig.h:133: #if defined(__SSE4_2__) || defined(__SSE4__) Are there compilers where __SSE4_2__ isn't defined but __SSE4__ is for SSE 4.2? Just curious about the need for the extra condition here. Clang and GCC's immintrin.h just seem to check __SSE4_2__. https://codereview.chromium.org/272503006/diff/1/src/opts/opts_check_x86.cpp File src/opts/opts_check_x86.cpp (right): https://codereview.chromium.org/272503006/diff/1/src/opts/opts_check_x86.cpp#... src/opts/opts_check_x86.cpp:107: static inline bool verifySIMDLevel(int minLevel) { verify reads me to me like an assert. Can we name this supports_simd, or even just supported, so it reads like if (supported(SK_CPU_SSE_LEVEL_SSE2)) { ... } https://codereview.chromium.org/272503006/diff/1/src/opts/opts_check_x86.cpp#... src/opts/opts_check_x86.cpp:111: #if defined(SK_DISABLE_RUNTIME_SIMD_CHECK) Just for parsimony, until we get another reason to define SK_DISABLE_RUNTIME_SIMD_CHECK, I'd prefer to keep the explicit #ifdef SK_BUILD_FOR_ANDROID_FRAMEWORK test and note all encapsulated inside here. Don't want someone to start relying on the accidental feature of being able to define SK_DISABLE_RUNTIME_SIMD_CHECK.
https://codereview.chromium.org/272503006/diff/1/include/core/SkPreConfig.h File include/core/SkPreConfig.h (right): https://codereview.chromium.org/272503006/diff/1/include/core/SkPreConfig.h#n... include/core/SkPreConfig.h:133: #if defined(__SSE4_2__) || defined(__SSE4__) On 2014/05/07 17:34:41, mtklein wrote: > Are there compilers where __SSE4_2__ isn't defined but __SSE4__ is for SSE 4.2? > Just curious about the need for the extra condition here. Clang and GCC's > immintrin.h just seem to check __SSE4_2__. As far as I can remember gcc used to set __SSE4__ as well, if you specified -msse4. This does not seem to be the case with the current gcc version, though. There's also a fair amount of hits in code, if you google "__SSE4__". They might have been set manually, though. I see no problem with removing this. https://codereview.chromium.org/272503006/diff/1/include/core/SkPreConfig.h#n... include/core/SkPreConfig.h:133: #if defined(__SSE4_2__) || defined(__SSE4__) On 2014/05/07 17:34:41, mtklein wrote: > Are there compilers where __SSE4_2__ isn't defined but __SSE4__ is for SSE 4.2? > Just curious about the need for the extra condition here. Clang and GCC's > immintrin.h just seem to check __SSE4_2__. Done. https://codereview.chromium.org/272503006/diff/1/src/opts/opts_check_x86.cpp File src/opts/opts_check_x86.cpp (right): https://codereview.chromium.org/272503006/diff/1/src/opts/opts_check_x86.cpp#... src/opts/opts_check_x86.cpp:107: static inline bool verifySIMDLevel(int minLevel) { On 2014/05/07 17:34:41, mtklein wrote: > verify reads me to me like an assert. Can we name this supports_simd, or even > just supported, so it reads like if (supported(SK_CPU_SSE_LEVEL_SSE2)) { ... } Done. Changed to supports_simd, since it's more future-proof. https://codereview.chromium.org/272503006/diff/1/src/opts/opts_check_x86.cpp#... src/opts/opts_check_x86.cpp:111: #if defined(SK_DISABLE_RUNTIME_SIMD_CHECK) On 2014/05/07 17:34:41, mtklein wrote: > Just for parsimony, until we get another reason to define > SK_DISABLE_RUNTIME_SIMD_CHECK, I'd prefer to keep the explicit #ifdef > SK_BUILD_FOR_ANDROID_FRAMEWORK test and note all encapsulated inside here. > Don't want someone to start relying on the accidental feature of being able to > define SK_DISABLE_RUNTIME_SIMD_CHECK. Done.
lgtm
The CQ bit was checked by henrik.smiding@intel.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/henrik.smiding@intel.com/272503006/20001
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 272503006-20001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Since the CL is editing public API, you must have an LGTM from one of: ('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org', 'bsalomon@google.com')
lgtm
The CQ bit was checked by mtklein@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/henrik.smiding@intel.com/272503006/20001
Message was sent while issue was closed.
Change committed as 14644
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/277593004/ by mtklein@google.com. The reason for reverting is: Windows builders breaking. :(.
On 2014/05/08 15:26:31, mtklein wrote: > A revert of this CL has been created in > https://codereview.chromium.org/277593004/ by mailto:mtklein@google.com. > > The reason for reverting is: Windows builders breaking. :(. Added a check for undefined SSE level. Could you please trigger some trybots for Windows build?
https://codereview.chromium.org/272503006/diff/40001/include/core/SkPreConfig.h File include/core/SkPreConfig.h (right): https://codereview.chromium.org/272503006/diff/40001/include/core/SkPreConfig... include/core/SkPreConfig.h:147: #ifndef SK_CPU_SSE_LEVEL Am I thinking right that this means this block isn't working now?
https://codereview.chromium.org/272503006/diff/40001/include/core/SkPreConfig.h File include/core/SkPreConfig.h (right): https://codereview.chromium.org/272503006/diff/40001/include/core/SkPreConfig... include/core/SkPreConfig.h:147: #ifndef SK_CPU_SSE_LEVEL On 2014/05/08 16:55:08, mtklein wrote: > Am I thinking right that this means this block isn't working now? It's probably working as intended when building 32-bit. But unlike gcc, visual studio doesn't seem to implicitly set the _M_IX86_FP to 'SSE2' when building for 64-bit. I'll add a check for 64-bit.
Thank you. Sorry for the delays. LGTM. Let's try this again...
The CQ bit was checked by mtklein@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/henrik.smiding@intel.com/272503006/60001
Message was sent while issue was closed.
Change committed as 14693 |
