Closed Bug 1073319 Opened 6 years ago Closed 5 years ago

VP9 performance regression due to broken libvpx AVX2 optimizations on Linux

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: rillian, Assigned: j)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Depends on: 1063356
Depends on: 1075183
Depends on: 1090632
Summary: Re-enable libvpx AVX2 optimizations on Linux → VP9 performance regression due to broken libvpx AVX2 optimizations on Linux
Depends on: 965151
Detect AVX2 toolchain support in configure and enable it if available.
Attachment #8626552 - Flags: review?(giles)
Assignee: nobody → j
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.
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)
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 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+
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)
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)
only tested this before the latest libvpx code landed.
media/libvpx/update.py still explicitly disabled avx2.
will have a look.
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)
and running update.py with avx2 enabled
Attachment #8634631 - Flags: review?(giles)
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
Attachment #8634630 - Flags: review?(giles) → review+
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+
can you push to try?
Flags: needinfo?(giles)
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)
(In reply to Ralph Giles (:rillian) from comment #15)
> but that blocks on bug 965151.

and bug 1175546
Depends on: gcc-4.8
Flags: needinfo?(j)
(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.
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
(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)
Seems to be sticking this time.
Flags: needinfo?(giles)
https://hg.mozilla.org/mozilla-central/rev/ee076319a032
https://hg.mozilla.org/mozilla-central/rev/de25c6e17a8d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.