Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1408003007:
Refactor SkBlurImageFilter_Opts.h. (Closed)
|
Created:
5 years, 1 month ago by Stephen White Modified:
5 years, 1 month ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionRefactor SkBlurImageFilter_Opts.h.
Refactor box_blur() into a single driver function which
SSE*, NEON and generic code paths can use. I've used macros
to do this in order to keep debug performance reasonable,
but it's fairly ugly. I'm open to other suggestions.
BUG=skia:
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
Committed: https://skia.googlesource.com/skia/+/963037c1214029dde691b89738be8212a5b22cdf
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 4
Patch Set 6 : Fix typo #Patch Set 7 : Changes per review #
Total comments: 5
Patch Set 8 : Win32 fix #Patch Set 9 : Remove prefetch specialization (use SK_PREFETCH everywhere) #Patch Set 10 : Put back the reinterpret_cast<> to appease Windows _mm_prefetch #Messages
Total messages: 28 (11 generated)
Description was changed from ========== Refactor SkBlurImageFilter_Opts.h. BUG=skia: CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot ========== to ========== Refactor SkBlurImageFilter_Opts.h. BUG=skia: CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot ==========
senorblanco@chromium.org changed reviewers: + mtklein@google.com, robertphillips@google.com
Description was changed from ========== Refactor SkBlurImageFilter_Opts.h. BUG=skia: CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot ========== to ========== Refactor SkBlurImageFilter_Opts.h. Refactor box_blur() into a single driver function which SSE*, NEON and generic code paths can use. I've used macros to do this in order to keep debug performance reasonable, but it's fairly ugly. I'm open to other suggestions. BUG=skia: CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot ==========
mtklein@: robertphillips@: PTAL. Thanks!
I am a fan. lgtm as it stands, with some suggestions. https://codereview.chromium.org/1408003007/diff/80001/src/opts/SkBlurImageFil... File src/opts/SkBlurImageFilter_opts.h (right): https://codereview.chromium.org/1408003007/diff/80001/src/opts/SkBlurImageFil... src/opts/SkBlurImageFilter_opts.h:21: static auto expand = [](int p) { Seems like these can just be functions now? As-is seems fine, maybe a bit unusual. https://codereview.chromium.org/1408003007/diff/80001/src/opts/SkBlurImageFil... src/opts/SkBlurImageFilter_opts.h:65: #define PREFETCH(p) _mm_prefetch(reinterpret_cast<const char*>(p), _MM_HINT_T0) // TODO(mtklein): make SK_PREFETCH work with MSVC, use that directly. ?
https://codereview.chromium.org/1408003007/diff/80001/src/opts/SkBlurImageFil... File src/opts/SkBlurImageFilter_opts.h (right): https://codereview.chromium.org/1408003007/diff/80001/src/opts/SkBlurImageFil... src/opts/SkBlurImageFilter_opts.h:21: static auto expand = [](int p) { On 2015/10/27 19:35:03, mtklein wrote: > Seems like these can just be functions now? As-is seems fine, maybe a bit > unusual. Done. Tangentially, I'm wondering if the new macros could actually be lambdas, but I'm no expert with these new-fangled^H^H^H^H^H^H^H^H^H^H^Hfour-year-old C++ features. https://codereview.chromium.org/1408003007/diff/80001/src/opts/SkBlurImageFil... src/opts/SkBlurImageFilter_opts.h:65: #define PREFETCH(p) _mm_prefetch(reinterpret_cast<const char*>(p), _MM_HINT_T0) On 2015/10/27 19:35:03, mtklein wrote: > // TODO(mtklein): make SK_PREFETCH work with MSVC, use that directly. ? Done.
The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1408003007/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1408003007/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...) Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
On 2015/10/27 19:47:21, Stephen White wrote: > https://codereview.chromium.org/1408003007/diff/80001/src/opts/SkBlurImageFil... > File src/opts/SkBlurImageFilter_opts.h (right): > > https://codereview.chromium.org/1408003007/diff/80001/src/opts/SkBlurImageFil... > src/opts/SkBlurImageFilter_opts.h:21: static auto expand = [](int p) { > On 2015/10/27 19:35:03, mtklein wrote: > > Seems like these can just be functions now? As-is seems fine, maybe a bit > > unusual. > > Done. > > Tangentially, I'm wondering if the new macros could actually be lambdas, but I'm > no expert with these new-fangled^H^H^H^H^H^H^H^H^H^H^Hfour-year-old C++ > features. Yeah, they probably could. I think the macros are just fine for now though. Let's look at it all again once work settles down on this file.
https://codereview.chromium.org/1408003007/diff/120001/src/opts/SkBlurImageFi... File src/opts/SkBlurImageFilter_opts.h (right): https://codereview.chromium.org/1408003007/diff/120001/src/opts/SkBlurImageFi... src/opts/SkBlurImageFilter_opts.h:48: static inline __m128i _mm_mullo_epi32(__m128i a, __m128i b) { Annoyingly MSVC makes all the intrinsics visible. It's hard to hold that against them, as it means you can use them all without any particular compile-time flags, which is nice. Anyway seems fine to just rename this, e.g. `mm_mullo_epi32_shim`. https://codereview.chromium.org/1408003007/diff/120001/src/opts/SkBlurImageFi... src/opts/SkBlurImageFilter_opts.h:65: // TODO(mtklein): make SK_PREFETCH work with MSVC, use that directly. ? should be good to go now
The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1408003007/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1408003007/160001
https://codereview.chromium.org/1408003007/diff/120001/src/opts/SkBlurImageFi... File src/opts/SkBlurImageFilter_opts.h (right): https://codereview.chromium.org/1408003007/diff/120001/src/opts/SkBlurImageFi... src/opts/SkBlurImageFilter_opts.h:48: static inline __m128i _mm_mullo_epi32(__m128i a, __m128i b) { On 2015/10/27 20:24:34, mtklein wrote: > Annoyingly MSVC makes all the intrinsics visible. It's hard to hold that > against them, as it means you can use them all without any particular > compile-time flags, which is nice. > > Anyway seems fine to just rename this, e.g. `mm_mullo_epi32_shim`. Done. https://codereview.chromium.org/1408003007/diff/120001/src/opts/SkBlurImageFi... src/opts/SkBlurImageFilter_opts.h:65: // TODO(mtklein): make SK_PREFETCH work with MSVC, use that directly. ? On 2015/10/27 20:24:34, mtklein wrote: > should be good to go now Fixed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
https://codereview.chromium.org/1408003007/diff/120001/src/opts/SkBlurImageFi... File src/opts/SkBlurImageFilter_opts.h (right): https://codereview.chromium.org/1408003007/diff/120001/src/opts/SkBlurImageFi... src/opts/SkBlurImageFilter_opts.h:65: // TODO(mtklein): make SK_PREFETCH work with MSVC, use that directly. ? On 2015/10/27 21:12:16, Stephen White wrote: > On 2015/10/27 20:24:34, mtklein wrote: > > should be good to go now > > Fixed. Oh boo, not compiling on Windows. I guess this is the only place we use SK_PREFETCH...
The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1408003007/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1408003007/180001
On 2015/10/27 21:15:12, mtklein wrote: > https://codereview.chromium.org/1408003007/diff/120001/src/opts/SkBlurImageFi... > File src/opts/SkBlurImageFilter_opts.h (right): > > https://codereview.chromium.org/1408003007/diff/120001/src/opts/SkBlurImageFi... > src/opts/SkBlurImageFilter_opts.h:65: // TODO(mtklein): make SK_PREFETCH work > with MSVC, use that directly. ? > On 2015/10/27 21:12:16, Stephen White wrote: > > On 2015/10/27 20:24:34, mtklein wrote: > > > should be good to go now > > > > Fixed. > > Oh boo, not compiling on Windows. I guess this is the only place we use > SK_PREFETCH... I've put the reinterpret_cast back in for now. Maybe it should be in the header?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by senorblanco@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com Link to the patchset: https://codereview.chromium.org/1408003007/#ps180001 (title: "Put back the reinterpret_cast<> to appease Windows _mm_prefetch")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1408003007/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1408003007/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://skia.googlesource.com/skia/+/963037c1214029dde691b89738be8212a5b22cdf |
