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

Issue 1314763007: SkColorCubeFilter_opts: start with a statically-initializable zero. (Closed)

Created:
5 years, 3 months ago by mtklein_C
Modified:
5 years, 3 months ago
Reviewers:
mtklein, Noel Gordon
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

SkColorCubeFilter_opts: start with a statically-initializable zero. SkPMFloat(0) and SkPMFloat(0,0,0,0) end up with the same value, but the first goes through math to get there. The second is a lot more transparent to the compiler, and should compile all the way down to just `xorps xmmN,xmmN` or even be optimized away. Didn't measure any additional benefit from hoisting the zero outside the loop and writing `SkPMFloat color = zero;`. Perf win is <2%. BUG=skia: Committed: https://skia.googlesource.com/skia/+/435af2f736c85c3274a0c6760a3523810750d237

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -1 line) Patch
M src/opts/SkColorCubeFilter_opts.h View 1 chunk +1 line, -1 line 1 comment Download

Messages

Total messages: 10 (2 generated)
mtklein_C
5 years, 3 months ago (2015-08-27 13:25:01 UTC) #2
Noel Gordon
https://codereview.chromium.org/1314763007/diff/1/src/opts/SkColorCubeFilter_opts.h File src/opts/SkColorCubeFilter_opts.h (right): https://codereview.chromium.org/1314763007/diff/1/src/opts/SkColorCubeFilter_opts.h#newcode54 src/opts/SkColorCubeFilter_opts.h:54: SkPMFloat color(0,0,0,0); My earlier test patches did it this ...
5 years, 3 months ago (2015-08-27 13:31:50 UTC) #3
Noel Gordon
LGTM. Hoisting the zero might be a small win. Bigger fish to fry.
5 years, 3 months ago (2015-08-27 13:33:00 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1314763007/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1314763007/1
5 years, 3 months ago (2015-08-27 13:39:29 UTC) #6
commit-bot: I haz the power
Note for Reviewers: The CQ is waiting for an approval. If you believe that the ...
5 years, 3 months ago (2015-08-27 13:39:30 UTC) #7
mtklein
wtf, lgtm
5 years, 3 months ago (2015-08-27 13:45:35 UTC) #8
commit-bot: I haz the power
Committed patchset #1 (id:1) as https://skia.googlesource.com/skia/+/435af2f736c85c3274a0c6760a3523810750d237
5 years, 3 months ago (2015-08-27 13:46:06 UTC) #9
Noel Gordon
5 years, 3 months ago (2015-08-27 13:46:25 UTC) #10
Message was sent while issue was closed.
On 2015/08/27 13:45:35, mtklein wrote:
> wtf, lgtm

:)

Powered by Google App Engine
This is Rietveld 408576698