Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
239453010:
Properly enable S32_D16_filter_DX_SSE2 optimization. (Closed)
|
Created:
6 years, 8 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. |
DescriptionProperly enable S32_D16_filter_DX_SSE2 optimization.
Currently, the S32_D16_filter_DX_SSE2 optimization is only used in
configurations where the maximum SSE level is SSE2.
This patch enables it for higher levels, as well as fixing a color
conversion bug when the subpixels are converted into RGB565 format.
Also, refactored the function a bit, to make future modifications
less error-prone.
Author: henrik.smiding@intel.com
Signed-off-by: Henrik Smiding <henrik.smiding@intel.com>;
Committed: http://code.google.com/p/skia/source/detail?r=14333
Committed: http://code.google.com/p/skia/source/detail?r=14403
Patch Set 1 #
Total comments: 1
Patch Set 2 : Refactoring of code. #
Total comments: 3
Patch Set 3 : Removed unnecessary comments #Patch Set 4 : Fixed RGB inversion #
Total comments: 2
Patch Set 5 : Removed obsolete code #Messages
Total messages: 24 (0 generated)
Developers! Would you kindly use your vast knowledge and review this tiny patch, like the engineers of old!
https://codereview.chromium.org/239453010/diff/1/src/opts/opts_check_SSE2.cpp File src/opts/opts_check_SSE2.cpp (right): https://codereview.chromium.org/239453010/diff/1/src/opts/opts_check_SSE2.cpp... src/opts/opts_check_SSE2.cpp:125: void SkBitmapProcState::platformProcs() { I can see how easy this was to mess up before. Can we make it even clearer? I'm still having trouble identifying all the procs we want to set and following through all the paths they can take. Maybe something like if (hasSSE2) { fill in all SSE2 procs here } if (hasSSSE3) { fill in all SSSE3 procs here (overwriting our previous write of fSampleProc32) } ?
Second Mike's suggestion.
On 2014/04/16 16:40:38, mtklein wrote: > https://codereview.chromium.org/239453010/diff/1/src/opts/opts_check_SSE2.cpp > File src/opts/opts_check_SSE2.cpp (right): > > https://codereview.chromium.org/239453010/diff/1/src/opts/opts_check_SSE2.cpp... > src/opts/opts_check_SSE2.cpp:125: void SkBitmapProcState::platformProcs() { > I can see how easy this was to mess up before. Can we make it even clearer? > I'm still having trouble identifying all the procs we want to set and following > through all the paths they can take. > > Maybe something like > > if (hasSSE2) { > fill in all SSE2 procs here > } > > if (hasSSSE3) { > fill in all SSSE3 procs here (overwriting our previous write of fSampleProc32) > } > > ? The if-statements check the same variable as they modify. If we first check SSE2 and then SSSE3, fSampleProc32 might have been modified when we reach the SSSE3 checks. If so, the function comparisons agains fSampleProc32 for SSSE3 will not be true. I'm working on a larger cleanup of the opts_check file for x86 right now. I took the entire function from this cleanup work, and moved into this patch. It has an early exit check for SSE2, and a more clear path.
> The if-statements check the same variable as they modify. > If we first check SSE2 and then SSSE3, fSampleProc32 might have been modified > when we reach the SSSE3 checks. If so, the function comparisons agains > fSampleProc32 for SSSE3 will not be true. Ah, how gross. Thank you for the refactoring that you were able to do. This LGTM.
https://codereview.chromium.org/239453010/diff/20001/src/opts/opts_check_SSE2... File src/opts/opts_check_SSE2.cpp (right): https://codereview.chromium.org/239453010/diff/20001/src/opts/opts_check_SSE2... src/opts/opts_check_SSE2.cpp:132: if (fSampleProc32 == S32_opaque_D32_filter_DX) { // Check S32_opaque_D32_filter_DX Nit: '/* Check fSampleProc32 */' and similar block-heading comments might be useful, but these line-ending comments strike me as useless noise? The point of the comment shouldn't be *what* the code does if that's obvious by inspecition (which I believe it is here), but whether *why* the code does something. I'd like to increase the comment density in Skia, but not at the expense of comment quality. Other reviewers?
https://codereview.chromium.org/239453010/diff/20001/src/opts/opts_check_SSE2... File src/opts/opts_check_SSE2.cpp (right): https://codereview.chromium.org/239453010/diff/20001/src/opts/opts_check_SSE2... src/opts/opts_check_SSE2.cpp:132: if (fSampleProc32 == S32_opaque_D32_filter_DX) { // Check S32_opaque_D32_filter_DX On 2014/04/22 09:47:28, tomhudson wrote: > Nit: '/* Check fSampleProc32 */' and similar block-heading comments might be > useful, but these line-ending comments strike me as useless noise? The point of > the comment shouldn't be *what* the code does if that's obvious by inspecition > (which I believe it is here), but whether *why* the code does something. > > I'd like to increase the comment density in Skia, but not at the expense of > comment quality. Other reviewers? I'll remove the four end-of-line comments. I only added them to make it slightly easier to find the correct location to put future optimizations.
https://codereview.chromium.org/239453010/diff/20001/src/opts/opts_check_SSE2... File src/opts/opts_check_SSE2.cpp (right): https://codereview.chromium.org/239453010/diff/20001/src/opts/opts_check_SSE2... src/opts/opts_check_SSE2.cpp:132: if (fSampleProc32 == S32_opaque_D32_filter_DX) { // Check S32_opaque_D32_filter_DX On 2014/04/22 09:47:28, tomhudson wrote: > Nit: '/* Check fSampleProc32 */' and similar block-heading comments might be > useful, but these line-ending comments strike me as useless noise? The point of > the comment shouldn't be *what* the code does if that's obvious by inspecition > (which I believe it is here), but whether *why* the code does something. > > I'd like to increase the comment density in Skia, but not at the expense of > comment quality. Other reviewers? Done.
lgtm Thanks! Nobody but MikeK has spoken up, so I'm going to assume this is good to go.
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/henrik.smiding@intel.com/239453010/40001
Message was sent while issue was closed.
Change committed as 14333
Message was sent while issue was closed.
On 2014/04/23 19:26:17, I haz the power (commit-bot) wrote: > Change committed as 14333 Could this CL be responsible for RGBA/BGRA GM failures? e.g. http://chromium-skia-gm.commondatastorage.googleapis.com/gm/bitmap-64bitMD5/d...
Message was sent while issue was closed.
On 2014/04/23 19:54:05, bsalomon wrote: > On 2014/04/23 19:26:17, I haz the power (commit-bot) wrote: > > Change committed as 14333 > > Could this CL be responsible for RGBA/BGRA GM failures? > > e.g. > > http://chromium-skia-gm.commondatastorage.googleapis.com/gm/bitmap-64bitMD5/d... Seems to be the case.
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/246393013/ by bsalomon@google.com. The reason for reverting is: Broke GMs in 565 mode. To repro: out/Debug/gm --match filterbitmap_image_mandrill -w . --config 565 open filterbitmap_image_mandrill_512.png_565.png.
Message was sent while issue was closed.
On 2014/04/23 20:07:00, bsalomon wrote: > A revert of this CL has been created in > https://codereview.chromium.org/246393013/ by mailto:bsalomon@google.com. > > The reason for reverting is: Broke GMs in 565 mode. To repro: > out/Debug/gm --match filterbitmap_image_mandrill -w . --config 565 > open filterbitmap_image_mandrill_512.png_565.png. I didn't expect this. The function has been in Skia for a long time. It's still active for SSE2 builds, so something has to be done. We'll try to find the bug. If unsuccessful, it has to be disabled for SSE2 as well.
On 2014/04/23 20:06:05, bsalomon wrote: > On 2014/04/23 19:54:05, bsalomon wrote: > > On 2014/04/23 19:26:17, I haz the power (commit-bot) wrote: > > > Change committed as 14333 > > > > Could this CL be responsible for RGBA/BGRA GM failures? > > > > e.g. > > > > > http://chromium-skia-gm.commondatastorage.googleapis.com/gm/bitmap-64bitMD5/d... > > Seems to be the case. Found the bug and fixed it. Passed the test as well. The monkey is red once again. Please take a look.
On 2014/04/25 17:00:19, henrik.smiding wrote: > Found the bug and fixed it. Passed the test as well. The monkey is red once > again. > Please take a look. Do you have any sense of how much performance we lose by going back to SkPixel32ToPixel16(dstColor)? Is it enough that we should consider specializing for common pixel formats?
On 2014/04/28 10:36:59, tomhudson wrote: > On 2014/04/25 17:00:19, henrik.smiding wrote: > > Found the bug and fixed it. Passed the test as well. The monkey is red once > > again. > > Please take a look. > > Do you have any sense of how much performance we lose by going back to > SkPixel32ToPixel16(dstColor)? > Is it enough that we should consider specializing for common pixel formats? According to our measurements, using skia bench, it was actually faster when using the SkPixel32ToPixel16 function, compared to the mixed SSE/x86 one. The number of operations are identical. I guess the compilers have improved since it was written. 50% improvement using SkPixel32ToPixel16 20% improvement using current Android-only color conversion.
https://codereview.chromium.org/239453010/diff/60001/src/opts/SkBitmapProcSta... File src/opts/SkBitmapProcState_opts_SSE2.cpp (right): https://codereview.chromium.org/239453010/diff/60001/src/opts/SkBitmapProcSta... src/opts/SkBitmapProcState_opts_SSE2.cpp:748: *colors++ = SkPixel32ToPixel16(dstColor); If this is indeed faster then lets remove the code below instead of just commenting it out.
https://codereview.chromium.org/239453010/diff/60001/src/opts/SkBitmapProcSta... File src/opts/SkBitmapProcState_opts_SSE2.cpp (right): https://codereview.chromium.org/239453010/diff/60001/src/opts/SkBitmapProcSta... src/opts/SkBitmapProcState_opts_SSE2.cpp:748: *colors++ = SkPixel32ToPixel16(dstColor); On 2014/04/28 13:05:11, djsollen wrote: > If this is indeed faster then lets remove the code below instead of just > commenting it out. 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/239453010/80001
Message was sent while issue was closed.
Change committed as 14403 |
