Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1130)

Issue 873553003: Revert of SSE4 opaque blend using intrinsics instead of assembly. (Closed)

Created:
5 years, 10 months ago by stephana
Modified:
5 years, 10 months ago
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

Revert of SSE4 opaque blend using intrinsics instead of assembly. (patchset #16 id:300001 of https://codereview.chromium.org/874863002/) Reason for revert: This causes a bug on the 'hittestpath' GM on MacMini 4,1 See: https://gold.skia.org/#/triage/hittestpath?head=0 for details. Original issue's description: > SSE4 opaque blend using intrinsics instead of assembly. > > Since we had such a hard time with the assembly versions of this blit (to the > point that we have them completely disabled everywhere), I thought I'd take > a shot at writing a version of the blit using intrinsics. > > The key feature of SSE4 we're exploiting is that we can use ptest (_mm_test*) > to skip the blend when the 16 src pixels we consider each loop are all opaque > or all transparent. _mm_shuffle_epi8 from SSSE3 also lends a hand to extract > all those alphas. > > It's worth looking to see if we can backport this type of logic to SSE2 using > _mm_movemask_epi8, or up to 32 pixels at a time using AVX. > > My local performance testing doesn't show this to be an unambiguous win > (there are probably microbenchmarks and SKPs where we'd be better off just > powering through the blend rather than looking at alphas), but the potential > does seem tantalizing enough to let skiaperf vet it on the bots. (< 1.0x is a win.) > > DM says it draws pixel perfect compare to the old code. > > Microbenchmarks: > bitmap_RGBA_8888_A_source_stripes_two 14us -> 14.4us 1.03x > bitmap_RGBA_8888_A_source_stripes_three 14.3us -> 14.5us 1.01x > bitmap_RGBA_8888_scale_bilerp 61.9us -> 62.2us 1.01x > bitmap_RGBA_8888_update_volatile_scale_rotate_bilerp 102us -> 101us 0.99x > bitmap_RGBA_8888_scale_rotate_bilerp 103us -> 101us 0.99x > bitmap_RGBA_8888_scale 18.4us -> 18.2us 0.99x > bitmap_RGBA_8888_A_scale_rotate_bicubic 71us -> 70us 0.99x > bitmap_RGBA_8888_update_scale_rotate_bilerp 103us -> 101us 0.99x > bitmap_RGBA_8888_A_scale_rotate_bilerp 112us -> 109us 0.98x > bitmap_RGBA_8888_update_volatile 5.72us -> 5.58us 0.98x > bitmap_RGBA_8888 5.73us -> 5.58us 0.97x > bitmap_RGBA_8888_update 5.78us -> 5.6us 0.97x > bitmap_RGBA_8888_A_scale_bilerp 70.7us -> 68us 0.96x > bitmap_RGBA_8888_A_scale_bicubic 23.7us -> 21.8us 0.92x > bitmap_RGBA_8888_A 13.9us -> 10.9us 0.78x > bitmap_RGBA_8888_A_source_opaque 14us -> 6.29us 0.45x > bitmap_RGBA_8888_A_source_transparent 14us -> 3.65us 0.26x > > Running over our ~70 SKP web page captures, this looks like we spend 0.7x > the time in S32A_Opaque_BlitRow compared to the SSE2 version, which should > be a decent predictor of real-world impact. > > BUG=chromium:399842 > > Committed: https://skia.googlesource.com/skia/+/04bc91b972417038fecfa87c484771eac2b9b785 > > CQ_EXTRA_TRYBOTS=client.skia:Test-Mac10.6-MacMini4.1-GeForce320M-x86_64-Release-Trybot > > Committed: https://skia.googlesource.com/skia/+/6dbfb21a6c88af6d94e8c823c3ad559f1a41b493 TBR=henrik.smiding@intel.com,mtklein@google.com,herb@google.com,reed@google.com,thakis@chromium.org,mtklein@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=chromium:399842 Committed: https://skia.googlesource.com/skia/+/4988891a1173cd405bf1c1dd3a3668c451f45e4c

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+972 lines, -80 lines) Patch
M gyp/opts.gypi View 1 chunk +0 lines, -1 line 0 comments Download
M src/opts/SkBlitRow_opts_SSE4.h View 1 chunk +19 lines, -4 lines 0 comments Download
D src/opts/SkBlitRow_opts_SSE4.cpp View 1 chunk +0 lines, -66 lines 0 comments Download
A src/opts/SkBlitRow_opts_SSE4_asm.S View 1 chunk +475 lines, -0 lines 0 comments Download
A src/opts/SkBlitRow_opts_SSE4_x64_asm.S View 1 chunk +472 lines, -0 lines 0 comments Download
M src/opts/SkColor_opts_SSE2.h View 1 chunk +1 line, -8 lines 0 comments Download
M src/opts/opts_check_x86.cpp View 1 chunk +5 lines, -1 line 0 comments Download

Messages

Total messages: 4 (0 generated)
stephana
Created Revert of SSE4 opaque blend using intrinsics instead of assembly.
5 years, 10 months ago (2015-02-02 17:52:00 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/873553003/1
5 years, 10 months ago (2015-02-02 17:52:31 UTC) #2
commit-bot: I haz the power
Committed patchset #1 (id:1) as https://skia.googlesource.com/skia/+/4988891a1173cd405bf1c1dd3a3668c451f45e4c
5 years, 10 months ago (2015-02-02 17:52:46 UTC) #3
stephana
5 years, 10 months ago (2015-02-02 18:02:31 UTC) #4
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/894083002/ by stephana@google.com.

The reason for reverting is: Reverted the wrong CL. .

Powered by Google App Engine
This is Rietveld 408576698