1073319 - VP9 performance regression due to broken libvpx AVX2 optimizations on Linux

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Ralph Giles (:rillian)]

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.

Comment 1

[Jan Gerber]

Attachment: 0001-Bug-1073319-Detect-avx2-toolchain-support-and-use-if.patch

Detect AVX2 toolchain support in configure and enable it if available.

Comment 2

[Ralph Giles (:rillian)]

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.

Comment 3

[Jan Gerber]

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 4

[Ralph Giles (:rillian)]

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.

Comment 5

[(not currently active) Ted Mielczarek]

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.

Comment 6

[Jan Gerber]

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?

Comment 7

[Ralph Giles (:rillian)]

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)

Comment 8

[Jan Gerber]

only tested this before the latest libvpx code landed.
media/libvpx/update.py still explicitly disabled avx2.
will have a look.

Comment 9

[Jan Gerber]

Attachment: Bug-1073319-P1.-Enable-AVX2-on-linux-update.py.patch

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

Comment 10

[Jan Gerber]

Attachment: Bug-1073319-P2.-Enable-AVX2-on-linux-libvpx.patch

and running update.py with avx2 enabled

Comment 11

[Ralph Giles (:rillian)]

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

Comment 12

[Ralph Giles (:rillian)]

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.

Comment 13

[Jan Gerber]

can you push to try?

Comment 14

[Ralph Giles (:rillian)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5cb24e27eae

Comment 15

[Ralph Giles (:rillian)]

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.

Comment 16

[Mike Hommey [:glandium]]

(In reply to Ralph Giles (:rillian) from comment #15)
> but that blocks on bug 965151.

and bug 1175546

Comment 17

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(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.

Comment 18

[Jan Gerber]

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?

Comment 19

[Jan Gerber]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9a1760cdfe9

Comment 20

[Jan Gerber]

(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?

Comment 21

[Ralph Giles (:rillian)]

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 22

[Ralph Giles (:rillian)]

Seems to be sticking this time.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ee076319a032
https://hg.mozilla.org/mozilla-central/rev/de25c6e17a8d