Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
169753004:
Enable the SSSE3 files to be built for Android when SSSE3 is not present. (Closed)
|
Created:
6 years, 10 months ago by djsollen Modified:
6 years, 10 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlesource.com/skia.git@master Visibility:
Public. |
DescriptionEnable the SSSE3 files to be built for Android when SSSE3 is not present.
Committed: http://code.google.com/p/skia/source/detail?r=13547
Committed: http://code.google.com/p/skia/source/detail?r=13583
Patch Set 1 #
Total comments: 9
Patch Set 2 : comments #Patch Set 3 : #Patch Set 4 : fix inverted logic #Patch Set 5 : #
Total comments: 2
Patch Set 6 : comments #Messages
Total messages: 19 (0 generated)
https://codereview.chromium.org/169753004/diff/1/include/core/SkPreConfig.h File include/core/SkPreConfig.h (right): https://codereview.chromium.org/169753004/diff/1/include/core/SkPreConfig.h#n... include/core/SkPreConfig.h:151: #define SK_CPU_SSE_LEVEL SK_CPU_SSE_LEVEL_SSE3 1. is there no way we can push this off to android.make, so that our header doesn't have to know about the latest snapshot of their comstraints? 2. Should this be guarded by building-for-x86? Seems like we're going to define SK_CPU_SSE_LEVEL on ARM builds...
https://codereview.chromium.org/169753004/diff/1/include/core/SkPreConfig.h File include/core/SkPreConfig.h (right): https://codereview.chromium.org/169753004/diff/1/include/core/SkPreConfig.h#n... include/core/SkPreConfig.h:140: #if defined(__x86_64__) || defined(_WIN64) Shouldn't we be able to just not do these platform checks? Won't the GCC/VisualStudio checks above have already set SK_CPU_SSE_LEVEL appropriately? https://codereview.chromium.org/169753004/diff/1/include/core/SkPreConfig.h#n... include/core/SkPreConfig.h:148: #if defined(SK_BUILD_FOR_ANDROID) Same here. Won't we just get this for free because we're always going to be passing -msse3? https://codereview.chromium.org/169753004/diff/1/src/opts/SkBitmapProcState_o... File src/opts/SkBitmapProcState_opts_SSSE3.cpp (right): https://codereview.chromium.org/169753004/diff/1/src/opts/SkBitmapProcState_o... src/opts/SkBitmapProcState_opts_SSSE3.cpp:727: #else Can you tack on // (SK_CPU_SSE_LEVEL < SK_CPU_SSE_LEVEL_SSSE3) ? https://codereview.chromium.org/169753004/diff/1/src/opts/SkBitmapProcState_o... src/opts/SkBitmapProcState_opts_SSSE3.cpp:738: sk_throw(); Some funky indents.
https://codereview.chromium.org/169753004/diff/1/include/core/SkPreConfig.h File include/core/SkPreConfig.h (right): https://codereview.chromium.org/169753004/diff/1/include/core/SkPreConfig.h#n... include/core/SkPreConfig.h:140: #if defined(__x86_64__) || defined(_WIN64) On 2014/02/21 19:24:31, mtklein wrote: > Shouldn't we be able to just not do these platform checks? Won't the > GCC/VisualStudio checks above have already set SK_CPU_SSE_LEVEL appropriately? I'll leave this untouched in the CL, but feel to create another to remove this. https://codereview.chromium.org/169753004/diff/1/include/core/SkPreConfig.h#n... include/core/SkPreConfig.h:148: #if defined(SK_BUILD_FOR_ANDROID) On 2014/02/21 19:24:31, mtklein wrote: > Same here. Won't we just get this for free because we're always going to be > passing -msse3? Agreed that we should rely on this. I'll dump the Android version I added. https://codereview.chromium.org/169753004/diff/1/src/opts/SkBitmapProcState_o... File src/opts/SkBitmapProcState_opts_SSSE3.cpp (right): https://codereview.chromium.org/169753004/diff/1/src/opts/SkBitmapProcState_o... src/opts/SkBitmapProcState_opts_SSSE3.cpp:727: #else On 2014/02/21 19:24:31, mtklein wrote: > Can you tack on // (SK_CPU_SSE_LEVEL < SK_CPU_SSE_LEVEL_SSSE3) ? Done. https://codereview.chromium.org/169753004/diff/1/src/opts/SkBitmapProcState_o... src/opts/SkBitmapProcState_opts_SSSE3.cpp:738: sk_throw(); On 2014/02/21 19:24:31, mtklein wrote: > Some funky indents. Done.
On 2014/02/21 19:38:22, djsollen wrote: > https://codereview.chromium.org/169753004/diff/1/include/core/SkPreConfig.h > File include/core/SkPreConfig.h (right): > > https://codereview.chromium.org/169753004/diff/1/include/core/SkPreConfig.h#n... > include/core/SkPreConfig.h:140: #if defined(__x86_64__) || defined(_WIN64) > On 2014/02/21 19:24:31, mtklein wrote: > > Shouldn't we be able to just not do these platform checks? Won't the > > GCC/VisualStudio checks above have already set SK_CPU_SSE_LEVEL appropriately? > > I'll leave this untouched in the CL, but feel to create another to remove this. > > https://codereview.chromium.org/169753004/diff/1/include/core/SkPreConfig.h#n... > include/core/SkPreConfig.h:148: #if defined(SK_BUILD_FOR_ANDROID) > On 2014/02/21 19:24:31, mtklein wrote: > > Same here. Won't we just get this for free because we're always going to be > > passing -msse3? > > Agreed that we should rely on this. I'll dump the Android version I added. > > https://codereview.chromium.org/169753004/diff/1/src/opts/SkBitmapProcState_o... > File src/opts/SkBitmapProcState_opts_SSSE3.cpp (right): > > https://codereview.chromium.org/169753004/diff/1/src/opts/SkBitmapProcState_o... > src/opts/SkBitmapProcState_opts_SSSE3.cpp:727: #else > On 2014/02/21 19:24:31, mtklein wrote: > > Can you tack on // (SK_CPU_SSE_LEVEL < SK_CPU_SSE_LEVEL_SSSE3) ? > > Done. > > https://codereview.chromium.org/169753004/diff/1/src/opts/SkBitmapProcState_o... > src/opts/SkBitmapProcState_opts_SSSE3.cpp:738: sk_throw(); > On 2014/02/21 19:24:31, mtklein wrote: > > Some funky indents. > > Done. sgtm. lgtm.
The CQ bit was checked by djsollen@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/djsollen@google.com/169753004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Presubmit check for 169753004-60001 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')
reed@ this looks like it needs your approval since it touches the include dir.
lgtm
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/djsollen@google.com/169753004/60001
Message was sent while issue was closed.
Change committed as 13547
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/175543004/ by scroggo@google.com. The reason for reverting is: Breaking the build. Non-android machines are calling sk_throw()..
ping. I would like everyone to take another look now that I've got a patch that works on all the platforms.
code LGTM. just some comment comments https://codereview.chromium.org/169753004/diff/300001/src/opts/SkBitmapProcSt... File src/opts/SkBitmapProcState_opts_SSSE3.cpp (right): https://codereview.chromium.org/169753004/diff/300001/src/opts/SkBitmapProcSt... src/opts/SkBitmapProcState_opts_SSSE3.cpp:17: #if SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSSE3 || !defined(SK_BUILD_FOR_ANDROID_FRAMEWORK) Might even want to swap the order of the arguments to ||. https://codereview.chromium.org/169753004/diff/300001/src/opts/SkBitmapProcSt... src/opts/SkBitmapProcState_opts_SSSE3.cpp:734: #else //SK_CPU_SSE_LEVEL >= SK_CPU_SSE_LEVEL_SSSE3 Update this?
The CQ bit was checked by djsollen@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/djsollen@google.com/169753004/320001
Message was sent while issue was closed.
Change committed as 13583 |
