Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1810183003:
Clean up SSSE3 and SSE4 stubs. (Closed)
DescriptionClean up SSSE3 and SSE4 stubs.
We added these stubs to work around OpenBSD's old compiler, which had
support for SSE2 but not SSSE3 or SSE4.
We now already have other unstubbed files that require SSSE3 and SSE4 compiler
support. All the compilers we support have SSSE3 and SSE4 support, and all the
way up to at least AVX2.
(Requiring C++11 has had some nice ripple effects...)
And, <immintrin.h> is already auto-included for these files, so no need for smmintrin or tmmintrin.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1810183003
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
Committed: https://skia.googlesource.com/skia/+/2b1b40e11afc41452b4d2f74cdebb1b6e6f7cc96
Patch Set 1 #
Messages
Total messages: 21 (7 generated)
Description was changed from ========== Clean up SSSE3 and SSE4 stubs. We added these stubs to work around OpenBSD's old compiler, which had support for SSE2 but not SSSE3 or SSE4. We now already have other unstubbed files that require SSSE3 and SSE4 compiler support. All the compilers we support have SSSE3 and SSE4 support, and all the way up to at least AVX2. (Requiring C++11 has had some nice ripple effects...) BUG=skia: ========== to ========== Clean up SSSE3 and SSE4 stubs. We added these stubs to work around OpenBSD's old compiler, which had support for SSE2 but not SSSE3 or SSE4. We now already have other unstubbed files that require SSSE3 and SSE4 compiler support. All the compilers we support have SSSE3 and SSE4 support, and all the way up to at least AVX2. (Requiring C++11 has had some nice ripple effects...) BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot ==========
Description was changed from ========== Clean up SSSE3 and SSE4 stubs. We added these stubs to work around OpenBSD's old compiler, which had support for SSE2 but not SSSE3 or SSE4. We now already have other unstubbed files that require SSSE3 and SSE4 compiler support. All the compilers we support have SSSE3 and SSE4 support, and all the way up to at least AVX2. (Requiring C++11 has had some nice ripple effects...) BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot ========== to ========== Clean up SSSE3 and SSE4 stubs. We added these stubs to work around OpenBSD's old compiler, which had support for SSE2 but not SSSE3 or SSE4. We now already have other unstubbed files that require SSSE3 and SSE4 compiler support. All the compilers we support have SSSE3 and SSE4 support, and all the way up to at least AVX2. (Requiring C++11 has had some nice ripple effects...) And, <immintrin.h> is already auto-included for these files, so no need for smmintrin or tmmintrin. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot ==========
The CQ bit was checked by mtklein@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/1810183003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1810183003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mtklein@chromium.org changed reviewers: + djsollen@google.com
lgtm
The CQ bit was checked by mtklein@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1810183003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1810183003/1
Message was sent while issue was closed.
Description was changed from ========== Clean up SSSE3 and SSE4 stubs. We added these stubs to work around OpenBSD's old compiler, which had support for SSE2 but not SSSE3 or SSE4. We now already have other unstubbed files that require SSSE3 and SSE4 compiler support. All the compilers we support have SSSE3 and SSE4 support, and all the way up to at least AVX2. (Requiring C++11 has had some nice ripple effects...) And, <immintrin.h> is already auto-included for these files, so no need for smmintrin or tmmintrin. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot ========== to ========== Clean up SSSE3 and SSE4 stubs. We added these stubs to work around OpenBSD's old compiler, which had support for SSE2 but not SSSE3 or SSE4. We now already have other unstubbed files that require SSSE3 and SSE4 compiler support. All the compilers we support have SSSE3 and SSE4 support, and all the way up to at least AVX2. (Requiring C++11 has had some nice ripple effects...) And, <immintrin.h> is already auto-included for these files, so no need for smmintrin or tmmintrin. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot Committed: https://skia.googlesource.com/skia/+/2b1b40e11afc41452b4d2f74cdebb1b6e6f7cc96 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://skia.googlesource.com/skia/+/2b1b40e11afc41452b4d2f74cdebb1b6e6f7cc96
Message was sent while issue was closed.
On 2016/03/18 15:10:36, commit-bot: I haz the power wrote: > Committed patchset #1 (id:1) as > https://skia.googlesource.com/skia/+/2b1b40e11afc41452b4d2f74cdebb1b6e6f7cc96 This appears to be breaking aosp_x86-userdebug: https://00e9e64bac9cff728a9fce673db128836eb59cc57bce037112-apidata.googleuser...
Message was sent while issue was closed.
On 2016/03/21 12:56:40, scroggo wrote: > On 2016/03/18 15:10:36, commit-bot: I haz the power wrote: > > Committed patchset #1 (id:1) as > > https://skia.googlesource.com/skia/+/2b1b40e11afc41452b4d2f74cdebb1b6e6f7cc96 > > This appears to be breaking aosp_x86-userdebug: > > https://00e9e64bac9cff728a9fce673db128836eb59cc57bce037112-apidata.googleuser... And, of course, that link is useless :(
Message was sent while issue was closed.
> And, of course, that link is useless :( Yeah. If this is breaking a build, it means we probably need to take a careful look at the flags it's building these files with. Odds are they're wrong.
Message was sent while issue was closed.
So I think we need to add the SSE4 checks back in, due to the following errors
on Android...
external/skia/src/opts/SkBlitRow_opts_SSE4.cpp:34:13: error: always_inline
function '_mm_testz_si128' requires target feature 'sse4.1', but would be
inlined into function 'S32A_Opaque_BlitRow32_SSE4' that is compiled without
support for 'sse4.1'
if (_mm_testz_si128(ORed, alphaMask)) {
^
external/skia/src/opts/SkBlitRow_opts_SSE4.cpp:39:13: error: always_inline
function '_mm_testc_si128' requires target feature 'sse4.1', but would be
inlined into function 'S32A_Opaque_BlitRow32_SSE4' that is compiled without
support for 'sse4.1'
if (_mm_testc_si128(ANDed, alphaMask)) {
Message was sent while issue was closed.
On 2016/03/22 13:17:37, djsollen wrote:
> So I think we need to add the SSE4 checks back in, due to the following errors
> on Android...
>
> external/skia/src/opts/SkBlitRow_opts_SSE4.cpp:34:13: error: always_inline
> function '_mm_testz_si128' requires target feature 'sse4.1', but would be
> inlined into function 'S32A_Opaque_BlitRow32_SSE4' that is compiled without
> support for 'sse4.1'
> if (_mm_testz_si128(ORed, alphaMask)) {
> ^
> external/skia/src/opts/SkBlitRow_opts_SSE4.cpp:39:13: error: always_inline
> function '_mm_testc_si128' requires target feature 'sse4.1', but would be
> inlined into function 'S32A_Opaque_BlitRow32_SSE4' that is compiled without
> support for 'sse4.1'
> if (_mm_testc_si128(ANDed, alphaMask)) {
I don't think so. I think we need to figure out why those files are not being
compiled with SSE 4.1.
If we put the stubs back, we can runtime detect SSE 4.1 and actually call those
stubs, crashing.
Message was sent while issue was closed.
We don't have a good way in the Android.mk files to selectively compile with different compiler options. So we are stuck with the default options for that system image. Do you have a way that we can accommodate for that?
Message was sent while issue was closed.
On 2016/03/22 13:42:45, djsollen wrote:
> We don't have a good way in the Android.mk files to selectively compile with
> different compiler options. So we are stuck with the default options for that
> system image. Do you have a way that we can accommodate for that?
What exactly can we change there? Are there docs I can read?
We might test whether the "target" attribute works with the compilers Android's
using now. It should be supported by GCC for a long time and by Clang recently.
That'd let us selectively turn on support for particular instruction sets
function-by-function.
E.g. {g++,clang++} test.cc && ./a.out, where this is test.cc:
#include <immintrin.h>
#include <stdio.h>
__attribute__((target("sse4.1")))
void f_sse41(uint32_t x) {
__m128i y = _mm_cvtepu8_epi32(_mm_cvtsi32_si128(x));
printf("sse4.1 %llu %llu\n", y[0], y[1]);
}
__attribute__((target("avx2")))
void f_avx2(uint64_t x) {
__m256i y = _mm256_cvtepu8_epi32(_mm_cvtsi64_si128(x));
printf("avx2 %llu %llu %llu %llu\n", y[0], y[1], y[2], y[3]);
}
int main(void) {
uint64_t x = 0x0045004400430042;
f_sse41(x);
f_avx2(x);
return 0;
}
That should compile, and print
sse4.1 66 67
avx2 66 67 68 69
If Clang's too old, we'll see something like
test.cc:12:16: warning: unknown attribute 'target' ignored
[-Wunknown-attributes]
__attribute__((target("sse4.1")))
If that happens this plan is screwed.
If Clang is new enough but its headers are not (weird, but possible), we'll see
errors like this:
test.cc:14:17: error: use of undeclared identifier '_mm_cvtepu8_epi32'
__m128i y = _mm_cvtepu8_epi32(_mm_cvtsi32_si128(x));
If that happens we can do dirty preprocessor things to make this work, but I'd
rather not sully anyone else's mind with what that'd look like unless we have to
go down that route.
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/1819223003/ by mtklein@google.com. The reason for reverting is: I've just had a better idea about how to fix this. Let's revert while I work on it.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
