Closed
Bug 1073319
Opened 10 years ago
Closed 9 years ago
VP9 performance regression due to broken libvpx AVX2 optimizations on Linux
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: rillian, Assigned: j)
References
Details
Attachments
(2 files, 1 obsolete file)
1.32 KB,
patch
|
rillian
:
review+
|
Details | Diff | Splinter Review |
66.84 KB,
patch
|
rillian
:
review+
|
Details | Diff | Splinter Review |
In bug 946639 we enabled AVX2 optimizations on linux. In bug 1063356 we temporarily disabled them. This bug is to re-enable them for the updated source code.
Updated•10 years ago
|
Summary: Re-enable libvpx AVX2 optimizations on Linux → VP9 performance regression due to broken libvpx AVX2 optimizations on Linux
Assignee | ||
Comment 1•9 years ago
|
||
Detect AVX2 toolchain support in configure and enable it if available.
Attachment #8626552 -
Flags: review?(giles)
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → j
Reporter | ||
Comment 2•9 years ago
|
||
Comment on attachment 8626552 [details] [diff] [review]
0001-Bug-1073319-Detect-avx2-toolchain-support-and-use-if.patch
Review of attachment 8626552 [details] [diff] [review]:
-----------------------------------------------------------------
::: configure.in
@@ +1470,5 @@
> + # Check for -mavx2 on $CC
> + AC_MSG_CHECKING([if toolchain supports -mavx2 option])
> + HAVE_TOOLCHAIN_SUPPORT_MAVX2=
> + _SAVE_CFLAGS=$CFLAGS
> + CFLAGS="$CFLAGS -mavx2"
How does this interact with HAVE_X86_AVX2 can we just use that?
I can never remember if -mavx2 uses avx2 in normal output (which would break on older machines) or just enables the intrinsics.
Assignee | ||
Comment 3•9 years ago
|
||
HAVE_X86_AVX2 checks for asm avx2 support, HAVE_TOOLCHAIN_SUPPORT_MAVX2 checks for avx2 intrinsics with the -mavx2 flag. Not sure if those two things are always supported at the same time, thats why I added another check.
-mavx2 is only added to the files that need it in media/libvpx/moz.build so should not break on older machines (it is already used on osx/windows this just adds it to linux builds)
Reporter | ||
Comment 4•9 years ago
|
||
Comment on attachment 8626552 [details] [diff] [review]
0001-Bug-1073319-Detect-avx2-toolchain-support-and-use-if.patch
Review of attachment 8626552 [details] [diff] [review]:
-----------------------------------------------------------------
Ok, sounds reasonable. Thanks. r=me for media review. This also needs build peer review. Adding Ted for that.
Attachment #8626552 -
Flags: review?(ted)
Attachment #8626552 -
Flags: review?(giles)
Attachment #8626552 -
Flags: review+
Comment 5•9 years ago
|
||
Comment on attachment 8626552 [details] [diff] [review]
0001-Bug-1073319-Detect-avx2-toolchain-support-and-use-if.patch
Review of attachment 8626552 [details] [diff] [review]:
-----------------------------------------------------------------
::: configure.in
@@ +1474,5 @@
> + CFLAGS="$CFLAGS -mavx2"
> + AC_TRY_COMPILE([#include <immintrin.h>],
> + [__m256i c = _mm256_set_epi32(1, 2, 3, 4, 5, 6, 7, 8);],
> + AC_MSG_RESULT([yes])
> + [HAVE_TOOLCHAIN_SUPPORT_MAVX2=1],
Naming this _MAVX2 is a little weird since you set it for MSVC as well below (which doesn't use -mavx2), but I'm not terribly broken up about it.
Attachment #8626552 -
Flags: review?(ted) → review+
Assignee | ||
Comment 6•9 years ago
|
||
Name was mostly in line with HAVE_TOOLCHAIN_SUPPORT_MSSE4_1 and friends that also get enabled for MSVC.
So would leave it at that.
rillian, time for a try push?
Flags: needinfo?(giles)
Reporter | ||
Comment 7•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=532dd585e762
Failure on B2G desktop linux64:
gecko/media/libvpx/vpx_dsp/x86/variance_avx2.c:47:28: error: 'vpx_get16x16var_avx2' undeclared (first use in this function)
Flags: needinfo?(giles)
Assignee | ||
Comment 8•9 years ago
|
||
only tested this before the latest libvpx code landed.
media/libvpx/update.py still explicitly disabled avx2.
will have a look.
Assignee | ||
Comment 9•9 years ago
|
||
making avx2 a compile time option would require to add 2 more targets with avx2.
-mavx2 was introduced in GCC 4.7, whats the minium version of GCC required right now?
replacing the patch with one to just enable avx2 in linux
Attachment #8626552 -
Attachment is obsolete: true
Attachment #8634630 -
Flags: review?(giles)
Assignee | ||
Comment 10•9 years ago
|
||
and running update.py with avx2 enabled
Attachment #8634631 -
Flags: review?(giles)
Reporter | ||
Comment 11•9 years ago
|
||
GCC 4.7 is the minimum required version, so this should be fine.
https://dxr.mozilla.org/mozilla-central/source/build/autoconf/toolchain.m4#110
Reporter | ||
Updated•9 years ago
|
Attachment #8634630 -
Flags: review?(giles) → review+
Reporter | ||
Comment 12•9 years ago
|
||
Comment on attachment 8634631 [details] [diff] [review]
Bug-1073319-P2.-Enable-AVX2-on-linux-libvpx.patch
Review of attachment 8634631 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks. Sorry for the review delay.
Attachment #8634631 -
Flags: review?(giles) → review+
Reporter | ||
Comment 14•9 years ago
|
||
Flags: needinfo?(giles)
Reporter | ||
Comment 15•9 years ago
|
||
4.7 is apparently not new enough. Valgrind ci job which uses "gcc-4.7.3-0moz1" fails on try:
media/libvpx/vp9/common/x86/vp9_loopfilter_intrin_avx2.c
vp9_loopfilter_intrin_sse2.o
{standard input}: Assembler messages:
{standard input}:173: Error: no such instruction: `vpbroadcastb %xmm8,%xmm8'
I'd suggest we bump the minimum to 4.8, or whatever the normal builds use, but that blocks on bug 965151.
Flags: needinfo?(j)
Comment 16•9 years ago
|
||
(In reply to Ralph Giles (:rillian) from comment #15)
> but that blocks on bug 965151.
and bug 1175546
(In reply to Ralph Giles (:rillian) from comment #15)
> 4.7 is apparently not new enough.
I'd settle fixing perf on 4.8 and retaining the slow path for 4.7.
Assignee | ||
Comment 18•9 years ago
|
||
tested with 4.7.3 here and it builds ok, so the error in the valgrind ci job might be related to the valgrind version or valgrind not using gcc 4.7.3. possibly this all works since bug 965151 landed?
No longer depends on: gcc-4.8
Assignee | ||
Comment 19•9 years ago
|
||
Assignee | ||
Comment 20•9 years ago
|
||
(In reply to Jan Gerber from comment #19)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9a1760cdfe9
All green this time around, looks like bug 965151 indeed fixed the valgrind build issues.
rillian, can you land it once more?
Flags: needinfo?(giles)
Reporter | ||
Comment 21•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee076319a0327bcef3a637a711116ba2516d5b62
Bug 1073319 - Enable AVX2 for libvpx on linux (update.py). r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/de25c6e17a8dbf86e5e83396ef1054c9a98f4b08
Bug 1073319 - Enable AVX2 for libvpx on linux (libvpx). r=rillian
Comment 23•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ee076319a032
https://hg.mozilla.org/mozilla-central/rev/de25c6e17a8d
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•