Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
202903004:
Xfermode: SSE2 implementation of multiply_modeproc (Closed)
|
Created:
6 years, 9 months ago by qiankun Modified:
6 years, 8 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlesource.com/skia.git@xfermode Visibility:
Public. |
DescriptionXfermode: SSE2 implementation of multiply_modeproc
This patch implements basics for Xfermode SSE optimization. Based on
these basics, SSE2 implementation of multiply_modeproc is provided. SSE2
implementation for other modes will come in future. With this patch
performance of Xfermode_Multiply will improve about 45%. Here are the
data on desktop i7-3770.
before:
Xfermode_Multiply 8888: cmsecs = 33.30 565: cmsecs = 45.65
after:
Xfermode_Multiply 8888: cmsecs = 17.18 565: cmsecs = 24.87
BUG=
Committed: http://code.google.com/p/skia/source/detail?r=14006
Committed: http://code.google.com/p/skia/source/detail?r=14050
Committed: http://code.google.com/p/skia/source/detail?r=14107
Patch Set 1 #Patch Set 2 : add SkGetPacked(A/R/G/B)32_SSE2 function #
Total comments: 10
Patch Set 3 : fix reviewer's comments #Patch Set 4 : fix try-bot #Patch Set 5 : fix try-bot #Patch Set 6 : rebase #Patch Set 7 : rebase #Patch Set 8 : fix build failure #Patch Set 9 : fix build with clang #Patch Set 10 : fix windows and arm build failure #Patch Set 11 : fix buildbot failure #
Messages
Total messages: 55 (0 generated)
PTAL, thanks!
On 2014/03/18 09:16:07, qiankun wrote: > PTAL, thanks! Ping.
Feel free to send out the other changes all at once. My focus on the SSE code is sort of bursty, so I think we can probably land a lot more if we've got the whole plan laid out. https://codereview.chromium.org/202903004/diff/20001/src/opts/SkColor_opts_SS... File src/opts/SkColor_opts_SSE2.h (right): https://codereview.chromium.org/202903004/diff/20001/src/opts/SkColor_opts_SS... src/opts/SkColor_opts_SSE2.h:13: static inline __m128i SkAlphaMulAlpha_SSE2(__m128i a, __m128i b) { Can you add on // See SkAlphaMulAlpha / SkMulDiv255Round ? Not really sure why we aliased them like that. Makes it hard to find. https://codereview.chromium.org/202903004/diff/20001/src/opts/SkXfermode_opts... File src/opts/SkXfermode_opts_SSE2.cpp (right): https://codereview.chromium.org/202903004/diff/20001/src/opts/SkXfermode_opts... src/opts/SkXfermode_opts_SSE2.cpp:13: prod = _mm_add_epi32(prod, _mm_set1_epi32(128)); // prod += 128; These little translate-back-to-C++ comments are very helpful. You don't need to do them, but I want to let you know I really appreciate them. It's going to make it a lot easier to get people understanding our SSE code. https://codereview.chromium.org/202903004/diff/20001/src/opts/SkXfermode_opts... src/opts/SkXfermode_opts_SSE2.cpp:131: if (count > 0) { I'm sure it doesn't hurt, but isn't this outer if {} superfluous? https://codereview.chromium.org/202903004/diff/20001/src/opts/SkXfermode_opts... src/opts/SkXfermode_opts_SSE2.cpp:265: SkXfermodeProc gSSE2XfermodeProcs1[] = { Think SSE2XfermodeProcs1 can wait until we have at least one? https://codereview.chromium.org/202903004/diff/20001/src/opts/SkXfermode_opts... File src/opts/SkXfermode_opts_SSE2.h (right): https://codereview.chromium.org/202903004/diff/20001/src/opts/SkXfermode_opts... src/opts/SkXfermode_opts_SSE2.h:23: void* fProcSIMD; I'd be much much happier if we could store this as SkXfermodeProcSIMD fProcSIMD, or at least as __m128i (*fProcSIMD)(__m128i src, __m128i dst). Is there something technical preventing us from doing this? Skia does sometimes resort to working with void*, but it's almost always as const void*, implicitly meaning a byte array. If we're forced to do anything else with void*, I'd want to document it heavily.
Thanks for your comments. I uploaded a new patch. PTAL. For "other changes all at once", do you mean several patches group similar type modes at once or a big patch contains all modes? https://codereview.chromium.org/202903004/diff/20001/src/opts/SkColor_opts_SS... File src/opts/SkColor_opts_SSE2.h (right): https://codereview.chromium.org/202903004/diff/20001/src/opts/SkColor_opts_SS... src/opts/SkColor_opts_SSE2.h:13: static inline __m128i SkAlphaMulAlpha_SSE2(__m128i a, __m128i b) { On 2014/03/27 14:55:17, mtklein wrote: > Can you add on // See SkAlphaMulAlpha / SkMulDiv255Round ? > > Not really sure why we aliased them like that. Makes it hard to find. Done. https://codereview.chromium.org/202903004/diff/20001/src/opts/SkXfermode_opts... File src/opts/SkXfermode_opts_SSE2.cpp (right): https://codereview.chromium.org/202903004/diff/20001/src/opts/SkXfermode_opts... src/opts/SkXfermode_opts_SSE2.cpp:13: prod = _mm_add_epi32(prod, _mm_set1_epi32(128)); // prod += 128; On 2014/03/27 14:55:17, mtklein wrote: > These little translate-back-to-C++ comments are very helpful. You don't need to > do them, but I want to let you know I really appreciate them. It's going to > make it a lot easier to get people understanding our SSE code. I am glad to hear that:) https://codereview.chromium.org/202903004/diff/20001/src/opts/SkXfermode_opts... src/opts/SkXfermode_opts_SSE2.cpp:131: if (count > 0) { On 2014/03/27 14:55:17, mtklein wrote: > I'm sure it doesn't hurt, but isn't this outer if {} superfluous? Thanks for pointing out this. I removed the superfluous if {} in both xfer32() and xfer16. https://codereview.chromium.org/202903004/diff/20001/src/opts/SkXfermode_opts... src/opts/SkXfermode_opts_SSE2.cpp:265: SkXfermodeProc gSSE2XfermodeProcs1[] = { On 2014/03/27 14:55:17, mtklein wrote: > Think SSE2XfermodeProcs1 can wait until we have at least one? That's fine. https://codereview.chromium.org/202903004/diff/20001/src/opts/SkXfermode_opts... File src/opts/SkXfermode_opts_SSE2.h (right): https://codereview.chromium.org/202903004/diff/20001/src/opts/SkXfermode_opts... src/opts/SkXfermode_opts_SSE2.h:23: void* fProcSIMD; On 2014/03/27 14:55:17, mtklein wrote: > I'd be much much happier if we could store this as SkXfermodeProcSIMD fProcSIMD, > or at least as __m128i (*fProcSIMD)(__m128i src, __m128i dst). Is there > something technical preventing us from doing this? > > Skia does sometimes resort to working with void*, but it's almost always as > const void*, implicitly meaning a byte array. If we're forced to do anything > else with void*, I'd want to document it heavily. I referred the arm NEON code previously. Now, I avoid using void* and use SkXfermodeProcSIMD instead. It works well.
On 2014/03/28 07:52:57, qiankun wrote: > Thanks for your comments. I uploaded a new patch. PTAL. > For "other changes all at once", do you mean several patches group similar type > modes at once or a big patch contains all modes? lgtm Lets do several, one CL for each transfer mode, unless they're all somehow building on shared code, in which case, everything at once. I'll be able to review each of them more quickly in isolation, but if they've got to come together then that's okay with me. I just don't want you to feel you have to send, review, submit, send, review, submit all in series. We can get some pipelining and parallelism going here if you like.
On 2014/03/28 13:05:44, mtklein wrote: > On 2014/03/28 07:52:57, qiankun wrote: > > Thanks for your comments. I uploaded a new patch. PTAL. > > For "other changes all at once", do you mean several patches group similar > type > > modes at once or a big patch contains all modes? > > lgtm > > Lets do several, one CL for each transfer mode, unless they're all somehow > building on shared code, in which case, everything at once. I'll be able to > review each of them more quickly in isolation, but if they've got to come > together then that's okay with me. I just don't want you to feel you have to > send, review, submit, send, review, submit all in series. We can get some > pipelining and parallelism going here if you like. I am clear now.
The CQ bit was checked by qiankun.miao@intel.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/qiankun.miao@intel.com/202903004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on Build-Mac10.8-Clang-x86-Release-Trybot for step(s) BuildSkiaLib http://skia-tree-status-staging.appspot.com/redirect/compile-buildbots/builds...
The CQ bit was checked by qiankun.miao@intel.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/qiankun.miao@intel.com/202903004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for step(s) BuildBench, BuildEverything, BuildGm, BuildSkiaLib, BuildTests, BuildTools http://skia-tree-status-staging.appspot.com/redirect/compile-buildbots/builds...
The CQ bit was checked by qiankun.miao@intel.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/qiankun.miao@intel.com/202903004/60002
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for src/opts/SkColor_opts_SSE2.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file src/opts/SkColor_opts_SSE2.h
Hunk #1 FAILED at 8.
Hunk #2 FAILED at 19.
Hunk #3 FAILED at 85.
3 out of 3 hunks FAILED -- saving rejects to file
src/opts/SkColor_opts_SSE2.h.rej
Patch: src/opts/SkColor_opts_SSE2.h
Index: src/opts/SkColor_opts_SSE2.h
diff --git a/src/opts/SkColor_opts_SSE2.h b/src/opts/SkColor_opts_SSE2.h
index
c61a26428c37636dcb1f458b970f7175dc2542c5..9622202df791ca0ce811433ae90d97c0b76567b6
100644
--- a/src/opts/SkColor_opts_SSE2.h
+++ b/src/opts/SkColor_opts_SSE2.h
@@ -8,8 +8,11 @@
#ifndef SkColor_opts_SSE2_DEFINED
#define SkColor_opts_SSE2_DEFINED
+#include "SkTypes.h"
+
#include <emmintrin.h>
+// See #define SkAlphaMulAlpha(a, b) SkMulDiv255Round(a, b) in SkXfermode.cpp.
static inline __m128i SkAlphaMulAlpha_SSE2(__m128i a, __m128i b) {
__m128i prod = _mm_mullo_epi16(a, b);
prod = _mm_add_epi32(prod, _mm_set1_epi32(128));
@@ -19,6 +22,26 @@ static inline __m128i SkAlphaMulAlpha_SSE2(__m128i a, __m128i
b) {
return prod;
}
+static inline __m128i SkGetPackedA32_SSE2(__m128i src) {
+ __m128i a = _mm_slli_epi32(src, (24 - SK_A32_SHIFT));
+ return _mm_srli_epi32(a, 24);
+}
+
+static inline __m128i SkGetPackedR32_SSE2(__m128i src) {
+ __m128i r = _mm_slli_epi32(src, (24 - SK_R32_SHIFT));
+ return _mm_srli_epi32(r, 24);
+}
+
+static inline __m128i SkGetPackedG32_SSE2(__m128i src) {
+ __m128i g = _mm_slli_epi32(src, (24 - SK_G32_SHIFT));
+ return _mm_srli_epi32(g, 24);
+}
+
+static inline __m128i SkGetPackedB32_SSE2(__m128i src) {
+ __m128i b = _mm_slli_epi32(src, (24 - SK_B32_SHIFT));
+ return _mm_srli_epi32(b, 24);
+}
+
static inline __m128i SkMul16ShiftRound_SSE2(__m128i a,
__m128i b, int shift) {
__m128i prod = _mm_mullo_epi16(a, b);
@@ -85,7 +108,8 @@ static inline __m128i SkPixel16ToPixel32_SSE2(__m128i src) {
return SkPackARGB32_SSE2(_mm_set1_epi32(0xFF), r, g, b);
}
-static inline __m128i SkPixel32ToPixel16_ToU16_SSE2(__m128i src_pixel1, __m128i
src_pixel2) {
+static inline __m128i SkPixel32ToPixel16_ToU16_SSE2(__m128i src_pixel1,
+ __m128i src_pixel2) {
// Calculate result r.
__m128i r1 = _mm_srli_epi32(src_pixel1,
SK_R32_SHIFT + (8 - SK_R16_BITS));
The CQ bit was checked by qiankun.miao@intel.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/qiankun.miao@intel.com/202903004/110001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on Build-Mac10.8-Clang-x86-Release-Trybot for step(s) BuildBench, BuildEverything http://skia-tree-status-staging.appspot.com/redirect/compile-buildbots/builds...
On 2014/03/31 04:36:40, I haz the power (commit-bot) wrote: > Retried try job too often on Build-Mac10.8-Clang-x86-Release-Trybot for step(s) > BuildBench, BuildEverything > http://skia-tree-status-staging.appspot.com/redirect/compile-buildbots/builds... Hi Mike, Could you help to see this build failure? The macro should be defined in SkTypes.h, but it seems compiler cannot recognize it. It built successfully at my machine. [21:32:09.079091] ../src/opts/SkXfermode_opts_SSE2.h:20:5: error: C++ requires a type specifier for all declarations [21:32:09.079213] SK_DEVELOPER_TO_STRING() [21:32:09.079311] ^~~~~~~~~~~~~~~~~~~~~~ [21:32:09.079407] ../src/opts/SkXfermode_opts_SSE2.h:20:29: error: expected ';' at end of declaration list [21:32:09.079514] SK_DEVELOPER_TO_STRING() [21:32:09.079612] ^ [21:32:09.079707] ; [21:32:09.079802] 2 errors generated.
On 2014/03/31 04:47:06, qiankun wrote: > On 2014/03/31 04:36:40, I haz the power (commit-bot) wrote: > > Retried try job too often on Build-Mac10.8-Clang-x86-Release-Trybot for > step(s) > > BuildBench, BuildEverything > > > http://skia-tree-status-staging.appspot.com/redirect/compile-buildbots/builds... > > Hi Mike, > > Could you help to see this build failure? The macro should be defined in > SkTypes.h, but it seems compiler cannot recognize it. It built successfully at > my machine. > > [21:32:09.079091] ../src/opts/SkXfermode_opts_SSE2.h:20:5: error: C++ requires a > type specifier for all declarations > [21:32:09.079213] SK_DEVELOPER_TO_STRING() > [21:32:09.079311] ^~~~~~~~~~~~~~~~~~~~~~ > [21:32:09.079407] ../src/opts/SkXfermode_opts_SSE2.h:20:29: error: expected ';' > at end of declaration list > [21:32:09.079514] SK_DEVELOPER_TO_STRING() > [21:32:09.079612] ^ > [21:32:09.079707] ; > [21:32:09.079802] 2 errors generated. Please ignore last message. I just fixed this build failure. Please review the new patch.
On 2014/03/31 06:36:44, qiankun wrote: > On 2014/03/31 04:47:06, qiankun wrote: > > On 2014/03/31 04:36:40, I haz the power (commit-bot) wrote: > > > Retried try job too often on Build-Mac10.8-Clang-x86-Release-Trybot for > > step(s) > > > BuildBench, BuildEverything > > > > > > http://skia-tree-status-staging.appspot.com/redirect/compile-buildbots/builds... > > > > Hi Mike, > > > > Could you help to see this build failure? The macro should be defined in > > SkTypes.h, but it seems compiler cannot recognize it. It built successfully at > > my machine. > > > > [21:32:09.079091] ../src/opts/SkXfermode_opts_SSE2.h:20:5: error: C++ requires > a > > type specifier for all declarations > > [21:32:09.079213] SK_DEVELOPER_TO_STRING() > > [21:32:09.079311] ^~~~~~~~~~~~~~~~~~~~~~ > > [21:32:09.079407] ../src/opts/SkXfermode_opts_SSE2.h:20:29: error: expected > ';' > > at end of declaration list > > [21:32:09.079514] SK_DEVELOPER_TO_STRING() > > [21:32:09.079612] ^ > > [21:32:09.079707] ; > > [21:32:09.079802] 2 errors generated. > > Please ignore last message. I just fixed this build failure. Please review the > new patch. 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/qiankun.miao@intel.com/202903004/130001
The CQ bit was checked by qiankun.miao@intel.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/qiankun.miao@intel.com/202903004/150001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch. Sending gyp/opts.gyp Sending src/core/SkXfermode.cpp Sending src/opts/SkBlitRow_opts_SSE2.cpp Sending src/opts/SkColor_opts_SSE2.h Adding src/opts/SkXfermode_opts_SSE2.cpp Adding src/opts/SkXfermode_opts_SSE2.h Sending src/opts/opts_check_SSE2.cpp Transmitting file data .......svn: Commit failed (details follow): svn: Server sent unexpected return value (401 Authorization Required) in response to MERGE request for '/svn/trunk'
On 2014/04/01 06:44:18, I haz the power (commit-bot) wrote: > Failed to apply the patch. > Sending gyp/opts.gyp > Sending src/core/SkXfermode.cpp > Sending src/opts/SkBlitRow_opts_SSE2.cpp > Sending src/opts/SkColor_opts_SSE2.h > Adding src/opts/SkXfermode_opts_SSE2.cpp > Adding src/opts/SkXfermode_opts_SSE2.h > Sending src/opts/opts_check_SSE2.cpp > Transmitting file data .......svn: Commit failed (details follow): > svn: Server sent unexpected return value (401 Authorization Required) in > response to MERGE request for '/svn/trunk' Hi Mike, Sorry to bother you again. I fixed the clang build failure. Please help to check in the patch. Thanks, Qiankun
On 2014/04/01 06:50:50, qiankun wrote: > On 2014/04/01 06:44:18, I haz the power (commit-bot) wrote: > > Failed to apply the patch. > > Sending gyp/opts.gyp > > Sending src/core/SkXfermode.cpp > > Sending src/opts/SkBlitRow_opts_SSE2.cpp > > Sending src/opts/SkColor_opts_SSE2.h > > Adding src/opts/SkXfermode_opts_SSE2.cpp > > Adding src/opts/SkXfermode_opts_SSE2.h > > Sending src/opts/opts_check_SSE2.cpp > > Transmitting file data .......svn: Commit failed (details follow): > > svn: Server sent unexpected return value (401 Authorization Required) in > > response to MERGE request for '/svn/trunk' > > Hi Mike, > > Sorry to bother you again. I fixed the clang build failure. Please help to check > in the patch. > > Thanks, > Qiankun Ah! Sorry about that. We had weird submit problems all day yesterday. I'll make sure this lands today.
The CQ bit was checked by mtklein@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/qiankun.miao@intel.com/202903004/150001
Message was sent while issue was closed.
Change committed as 14006
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/219243009/ by robertphillips@google.com. The reason for reverting is: Breaking builds.
Message was sent while issue was closed.
On Win8 (http://108.170.220.76:10117/builders/Build-Win8-VS2012-x86-Release/builds/224...): [07:04:07.064000] FAILED: ninja -t msvc -e environment.x86 -- "C:\Program Files (x86)\Microsoft Visual Studio 11.0\VC\bin\cl.exe" /nologo /showIncludes /FC @obj\src\opts\opts.SkXfermode_opts_SSE2.obj.rsp /c ..\..\src\opts\SkXfermode_opts_SSE2.cpp /Foobj\src\opts\opts.SkXfermode_opts_SSE2.obj /Fdopts.pdb [07:04:07.064000] c:\1\build-win8-vs2012-x86-release\build\skia\src\opts\skcolor_opts_sse2.h(63) : error C2719: 'b': formal parameter with __declspec(align('16')) won't be aligned [07:04:07.064000] c:\1\build-win8-vs2012-x86-release\build\skia\src\opts\skxfermode_opts_sse2.h(16) : warning C4373: 'SkSSE2ProcCoeffXfermode::xfer16': virtual function overrides 'SkProcXfermode::xfer16', previous versions of the compiler did not override when parameters only differed by const/volatile qualifiers [07:04:07.064000] c:\1\build-win8-vs2012-x86-release\build\skia\include\core\skxfermode.h(263) : see declaration of 'SkProcXfermode::xfer16' [07:04:07.064000] c:\1\build-win8-vs2012-x86-release\build\skia\src\opts\skxfermode_opts_sse2.cpp(48) : error C2719: 'da': formal parameter with __declspec(align('16')) won't be aligned [07:04:07.064000] FAILED: ninja -t msvc -e environment.x86 -- "C:\Program Files (x86)\Microsoft Visual Studio 11.0\VC\bin\cl.exe" /nologo /showIncludes /FC @obj\src\opts\opts.SkBlitRow_opts_SSE2.obj.rsp /c ..\..\src\opts\SkBlitRow_opts_SSE2.cpp /Foobj\src\opts\opts.SkBlitRow_opts_SSE2.obj /Fdopts.pdb [07:04:07.064000] c:\1\build-win8-vs2012-x86-release\build\skia\src\opts\skcolor_opts_sse2.h(63) : error C2719: 'b': formal parameter with __declspec(align('16')) won't be aligned [07:04:07.564000] FAILED: ninja -t msvc -e environment.x86 -- "C:\Program Files (x86)\Microsoft Visual Studio 11.0\VC\bin\cl.exe" /nologo /showIncludes /FC @obj\src\core\core.SkXfermode.obj.rsp /c ..\..\src\core\SkXfermode.cpp /Foobj\src\core\core.SkXfermode.obj /Fdcore.pdb [07:04:07.564000] c:\1\build-win8-vs2012-x86-release\build\skia\src\opts\skxfermode_opts_sse2.h(27) : error C2220: warning treated as error - no 'object' file generated [07:04:07.564000] c:\1\build-win8-vs2012-x86-release\build\skia\src\opts\skxfermode_opts_sse2.h(16) : warning C4373: 'SkSSE2ProcCoeffXfermode::xfer16': virtual function overrides 'SkProcXfermode::xfer16', previous versions of the compiler did not override when parameters only differed by const/volatile qualifiers [07:04:07.564000] c:\1\build-win8-vs2012-x86-release\build\skia\include\core\skxfermode.h(263) : see declaration of 'SkProcXfermode::xfer16' On Ubuntu12 (http://108.170.220.76:10117/builders/Build-Ubuntu12-GCC-Arm7-Release-Nexus4/b...) [14:12:27.963893] In file included from ../../../../src/core/SkXfermode.cpp:11:0: [14:12:27.964001] ../../../../src/opts/SkXfermode_opts_SSE2.h:6:17: error: ISO C++ forbids declaration of '__m128i' with no type [-fpermissive] [14:12:27.964033] ../../../../src/opts/SkXfermode_opts_SSE2.h:6:17: error: typedef '__m128i' is initialized (use decltype instead) [14:12:27.964058] ../../../../src/opts/SkXfermode_opts_SSE2.h:6:19: error: 'SkXfermodeProcSIMD' was not declared in this scope [14:12:27.964082] ../../../../src/opts/SkXfermode_opts_SSE2.h:11:29: error: 'SkXfermodeProcSIMD' has not been declared [14:12:27.964116] In file included from ../../../../src/core/SkXfermode.cpp:11:0: [14:12:27.964147] ../../../../src/opts/SkXfermode_opts_SSE2.h:25:5: error: 'SkXfermodeProcSIMD' does not name a type [14:12:27.964181] In file included from ../../../../src/core/SkXfermode.cpp:11:0: [14:12:27.964245] ../../../../src/opts/SkXfermode_opts_SSE2.h: In constructor 'SkSSE2ProcCoeffXfermode::SkSSE2ProcCoeffXfermode(const ProcCoeff&, SkXfermode::Mode, int)': [14:12:27.964292] ../../../../src/opts/SkXfermode_opts_SSE2.h:12:33: error: class 'SkSSE2ProcCoeffXfermode' does not have any field named 'fProcSIMD'
Message was sent while issue was closed.
> On Ubuntu12 > (http://108.170.220.76:10117/builders/Build-Ubuntu12-GCC-Arm7-Release-Nexus4/b...) > > [14:12:27.963893] In file included from > ../../../../src/core/SkXfermode.cpp:11:0: > [14:12:27.964001] ../../../../src/opts/SkXfermode_opts_SSE2.h:6:17: error: ISO > C++ forbids declaration of '__m128i' with no type [-fpermissive] > [14:12:27.964033] ../../../../src/opts/SkXfermode_opts_SSE2.h:6:17: error: > typedef '__m128i' is initialized (use decltype instead) > [14:12:27.964058] ../../../../src/opts/SkXfermode_opts_SSE2.h:6:19: error: > 'SkXfermodeProcSIMD' was not declared in this scope > [14:12:27.964082] ../../../../src/opts/SkXfermode_opts_SSE2.h:11:29: error: > 'SkXfermodeProcSIMD' has not been declared > [14:12:27.964116] In file included from > ../../../../src/core/SkXfermode.cpp:11:0: > [14:12:27.964147] ../../../../src/opts/SkXfermode_opts_SSE2.h:25:5: error: > 'SkXfermodeProcSIMD' does not name a type > [14:12:27.964181] In file included from > ../../../../src/core/SkXfermode.cpp:11:0: > [14:12:27.964245] ../../../../src/opts/SkXfermode_opts_SSE2.h: In constructor > 'SkSSE2ProcCoeffXfermode::SkSSE2ProcCoeffXfermode(const ProcCoeff&, > SkXfermode::Mode, int)': > [14:12:27.964292] ../../../../src/opts/SkXfermode_opts_SSE2.h:12:33: error: > class 'SkSSE2ProcCoeffXfermode' does not have any field named 'fProcSIMD' Crucially that's Ubuntu 12.x building for a ARMv7. I think we're seeing here why the ARM code stored the function pointer as a void*; apparently SkXfermode_opts_SSE2.h is included even when building for non-SSE2 platforms.
On 2014/04/01 14:24:35, mtklein wrote: > > On Ubuntu12 > > > (http://108.170.220.76:10117/builders/Build-Ubuntu12-GCC-Arm7-Release-Nexus4/b...) > > > > [14:12:27.963893] In file included from > > ../../../../src/core/SkXfermode.cpp:11:0: > > [14:12:27.964001] ../../../../src/opts/SkXfermode_opts_SSE2.h:6:17: error: ISO > > C++ forbids declaration of '__m128i' with no type [-fpermissive] > > [14:12:27.964033] ../../../../src/opts/SkXfermode_opts_SSE2.h:6:17: error: > > typedef '__m128i' is initialized (use decltype instead) > > [14:12:27.964058] ../../../../src/opts/SkXfermode_opts_SSE2.h:6:19: error: > > 'SkXfermodeProcSIMD' was not declared in this scope > > [14:12:27.964082] ../../../../src/opts/SkXfermode_opts_SSE2.h:11:29: error: > > 'SkXfermodeProcSIMD' has not been declared > > [14:12:27.964116] In file included from > > ../../../../src/core/SkXfermode.cpp:11:0: > > [14:12:27.964147] ../../../../src/opts/SkXfermode_opts_SSE2.h:25:5: error: > > 'SkXfermodeProcSIMD' does not name a type > > [14:12:27.964181] In file included from > > ../../../../src/core/SkXfermode.cpp:11:0: > > [14:12:27.964245] ../../../../src/opts/SkXfermode_opts_SSE2.h: In constructor > > 'SkSSE2ProcCoeffXfermode::SkSSE2ProcCoeffXfermode(const ProcCoeff&, > > SkXfermode::Mode, int)': > > [14:12:27.964292] ../../../../src/opts/SkXfermode_opts_SSE2.h:12:33: error: > > class 'SkSSE2ProcCoeffXfermode' does not have any field named 'fProcSIMD' > > Crucially that's Ubuntu 12.x building for a ARMv7. I think we're seeing here > why the ARM code stored the function pointer as a void*; apparently > SkXfermode_opts_SSE2.h is included even when building for non-SSE2 platforms. I will add #if SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSE2 guard in SkXfermode.cpp to avoid building SkXfermode_opts_SSE2.h for ARM device. With this guards do I still need to change back to void* for the function pointer? For the windows build failure, I am working on it. Do you have any idea about it?
> I will add #if SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSE2 guard in SkXfermode.cpp > to avoid building SkXfermode_opts_SSE2.h for ARM device. With this guards do I > still need to change back to void* for the function pointer? Let's try your plan first. > For the windows build failure, I am working on it. Do you have any idea about > it? I have a hunch it's telling us it's not safe to pass __m128 by value as function parameters. Could be it can't guarantee they're still 16-byte aligned when they go through a calling convention? Try rewriting __m128 f(__m128, __m128) -> __m128 f(const __m128&, const __m128&)?
On 2014/04/02 12:28:44, mtklein wrote: > > I will add #if SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSE2 guard in > SkXfermode.cpp > > to avoid building SkXfermode_opts_SSE2.h for ARM device. With this guards do I > > still need to change back to void* for the function pointer? > Let's try your plan first. > > > For the windows build failure, I am working on it. Do you have any idea about > > it? > > I have a hunch it's telling us it's not safe to pass __m128 by value as function > parameters. Could be it can't guarantee they're still 16-byte aligned when they > go through a calling convention? Try rewriting __m128 f(__m128, __m128) -> > __m128 f(const __m128&, const __m128&)? Uploaded a new patch, PTAL.
On 2014/04/03 07:43:21, qiankun wrote: > On 2014/04/02 12:28:44, mtklein wrote: > > > I will add #if SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSE2 guard in > > SkXfermode.cpp > > > to avoid building SkXfermode_opts_SSE2.h for ARM device. With this guards do > I > > > still need to change back to void* for the function pointer? > > Let's try your plan first. > > > > > For the windows build failure, I am working on it. Do you have any idea > about > > > it? > > > > I have a hunch it's telling us it's not safe to pass __m128 by value as > function > > parameters. Could be it can't guarantee they're still 16-byte aligned when > they > > go through a calling convention? Try rewriting __m128 f(__m128, __m128) -> > > __m128 f(const __m128&, const __m128&)? > > Uploaded a new patch, PTAL. LGTM. Here we go 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/qiankun.miao@intel.com/202903004/160001
Message was sent while issue was closed.
Change committed as 14050
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/224253003/ by robertphillips@google.com. The reason for reverting is: It looks like serialization is broken. The serialize and pipe-cross-process tests are failing and turning (at least the Ubuntu12 and Win7) bots red.
Message was sent while issue was closed.
Here are the logs from the Ubuntu bot: http://108.170.220.120:10117/builders/Test-Ubuntu12-ShuttleA-GTX660-x86-Debug... http://108.170.220.120:10117/builders/Test-Ubuntu12-ShuttleA-GTX660-x86-Debug...
Message was sent while issue was closed.
Here's how to reproduce the issue: sync your repo to your CL (r14050) build it mkdir good bad diffs run: out\Debug\gm --serialize --mismatchPath bad --match xfermodes3 -w good --config 8888 rename the xfermodes3_8888-serialize.png in bad to xfermodes3_8888.png out\Debug\skdiff.exe good bad diffs open up diffs\index.html
On 2014/04/03 20:06:27, robertphillips wrote: > Here's how to reproduce the issue: > > sync your repo to your CL (r14050) > build it > mkdir good bad diffs > run: out\Debug\gm --serialize --mismatchPath bad --match xfermodes3 -w good > --config 8888 > rename the xfermodes3_8888-serialize.png in bad to xfermodes3_8888.png > out\Debug\skdiff.exe good bad diffs > open up diffs\index.html This issue is due to "#if SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSE2" in SkXfermode.cpp is not good enough to guard the related code. According my test, for Visual Studio the compiler can't find the macro declaration of "_M_IX86_FP" which is used to calculated "SK_CPU_SSE_LEVEL" even "EnableEnhancedInstructionSet" is set to "SSE2" in gyp file. I am seeking a safe way to identify if the compiler or platform support SSE2 during compiling the code.
On 2014/04/04 10:16:47, qiankun wrote: > On 2014/04/03 20:06:27, robertphillips wrote: > > Here's how to reproduce the issue: > > > > sync your repo to your CL (r14050) > > build it > > mkdir good bad diffs > > run: out\Debug\gm --serialize --mismatchPath bad --match xfermodes3 -w good > > --config 8888 > > rename the xfermodes3_8888-serialize.png in bad to xfermodes3_8888.png > > out\Debug\skdiff.exe good bad diffs > > open up diffs\index.html > > This issue is due to "#if SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSE2" in > SkXfermode.cpp is not good enough to guard the related code. According my test, > for Visual Studio the compiler can't find the macro declaration of "_M_IX86_FP" > which is used to calculated "SK_CPU_SSE_LEVEL" even > "EnableEnhancedInstructionSet" is set to "SSE2" in gyp file. I am seeking a safe > way to identify if the compiler or platform support SSE2 during compiling the > code. PTAL. I didn't find a better method to determine if SSE2 is available for all platforms, so change back to the original solution. Use void* function pointer to make core build successfully.
On 2014/04/09 06:59:13, qiankun wrote: > On 2014/04/04 10:16:47, qiankun wrote: > > On 2014/04/03 20:06:27, robertphillips wrote: > > > Here's how to reproduce the issue: > > > > > > sync your repo to your CL (r14050) > > > build it > > > mkdir good bad diffs > > > run: out\Debug\gm --serialize --mismatchPath bad --match xfermodes3 -w good > > > --config 8888 > > > rename the xfermodes3_8888-serialize.png in bad to xfermodes3_8888.png > > > out\Debug\skdiff.exe good bad diffs > > > open up diffs\index.html > > > > This issue is due to "#if SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSE2" in > > SkXfermode.cpp is not good enough to guard the related code. According my > test, > > for Visual Studio the compiler can't find the macro declaration of > "_M_IX86_FP" > > which is used to calculated "SK_CPU_SSE_LEVEL" even > > "EnableEnhancedInstructionSet" is set to "SSE2" in gyp file. I am seeking a > safe > > way to identify if the compiler or platform support SSE2 during compiling the > > code. > > PTAL. I didn't find a better method to determine if SSE2 is available for all > platforms, so change back to the original solution. Use void* function pointer > to make core build successfully. Sad. LGTM. src/core/SkXfermode.cpp is probably not compiled with SSE2 unless on a 64-bit machine. That's why the guard there isn't working: inside SkXfermode.cpp, SK_CPU_SSE_LEVEL really is 0.
The CQ bit was checked by mtklein@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/qiankun.miao@intel.com/202903004/180001
Message was sent while issue was closed.
Change committed as 14107 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
