1061339 - Enable AVX2 in libvpx for VS2012+

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[:dmajor]

This was already done for libyuv in bug 1054811. Let's do the same for libvpx. (I don't see any other uses besides those two libs)

Comment 1

[xunxun]

Attachment: Bug 1061339 - Enable AVX2 in libvpx for VS2012+

I think the patch is very easy.

Comment 2

[xunxun]

Attachment: Enable AVX2 in libvpx for VS2012+

I think HAVE_X86_AVX2 is enough

Comment 3

[Makoto Kato [:m_kato]]

(In reply to xunxun from comment #2)
> Created attachment 8482755 [details] [diff] [review]
> Enable AVX2 in libvpx for VS2012+
> 
> I think HAVE_X86_AVX2 is enough

Not enouth.  You have to generate all headers such as vp9_rtcd_x86-win32-vs11.h by original source code.

Comment 4

[Makoto Kato [:m_kato]]

- configure into libvpx-1.3.0 can generate vpx_config.* for that platform.
- make after configure can generate vp9_rtcd.h, vp8_rtcd.h and vpx_scale_rtcd.h for that platform
- include these by vp8_rtcd.h, vp9_rtcd.h and vpx_config.asm and vpx_config.h

generated vp9_rtcd.h for VS2012+ will call AVX2 code.  So your patch never calls AVX2's intrinsic.

Comment 5

[Makoto Kato [:m_kato]]

also, vpx_rtcd.h is't used now.  It is bug 1061538

Comment 6

[xunxun]

(In reply to Makoto Kato (:m_kato) from comment #4)
> - configure into libvpx-1.3.0 can generate vpx_config.* for that platform.
> - make after configure can generate vp9_rtcd.h, vp8_rtcd.h and
> vpx_scale_rtcd.h for that platform
> - include these by vp8_rtcd.h, vp9_rtcd.h and vpx_config.asm and vpx_config.h
> 
> generated vp9_rtcd.h for VS2012+ will call AVX2 code.  So your patch never
> calls AVX2's intrinsic.

Can you take the bug?

I found that Mozilla in-tree libvpx 1.3.0 also lack vp9/encoder/x86/*avx2.c, and I have no AVX2 CPU, I can't test it.

Comment 7

[Makoto Kato [:m_kato]]

(In reply to xunxun from comment #6)

> I found that Mozilla in-tree libvpx 1.3.0 also lack vp9/encoder/x86/*avx2.c,
> and I have no AVX2 CPU, I can't test it.

That files are upstream only.

Comment 8

[Makoto Kato [:m_kato]]

Attachment: Add vs2012+ configuration

Comment 9

[xunxun]

Maybe we should add vp9_loopfilter_intrin_avx2 's CFLAGS -arch:AVX2?

Comment 10

[Makoto Kato [:m_kato]]

(In reply to xunxun from comment #9)
> Maybe we should add vp9_loopfilter_intrin_avx2 's CFLAGS -arch:AVX2?

It is unnecessary.  VS can compile AVX2 intrinsic without it.  gcc requires -mavx2 for it.

Comment 11

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8e2f5c99ede

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Makefile.in changes require build peer review. Please request post-facto review or backout.

Comment 13

[Makoto Kato [:m_kato]]

OK. I will backout soon.

Comment 14

[Makoto Kato [:m_kato]]

backed out
https://hg.mozilla.org/integration/mozilla-inbound/rev/a83356099127

Comment 15

[Makoto Kato [:m_kato]]

Comment on attachment 8483995 [details] [diff] [review]
Add vs2012+ configuration

No way to detect visual studio version on yasm.  So I want to add vc version for condition of yasm assembler.

Comment 16

[Makoto Kato [:m_kato]]

And, YASM's flags cannot apply DEFINES['xxx'] on moz.build.

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8483995 [details] [diff] [review]
Add vs2012+ configuration

Review of attachment 8483995 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the review latency - I was on holiday.

Comment 18

[xunxun]

Now with review+, will the patch commit to m-i/m-c?

Comment 19

[:dmajor]

The patch can be simplified a lot now that we require minimum VS2013.

Comment 20

[Makoto Kato [:m_kato]]

This need rebase and review again.

Comment 21

[Makoto Kato [:m_kato]]

Attachment: Part 1. Always use VS2013 target

Comment 22

[Makoto Kato [:m_kato]]

Attachment: Part 2. Build AVX code on all Windows build

Comment 23

[Makoto Kato [:m_kato]]

Comment on attachment 8565275 [details] [diff] [review]
Part 1. Always use VS2013 target

Minimal requirement of Windows MSVS build is 2013.  So we should use x86-win32-vs12 and x86_64-win64-vs12 as target.

Comment 24

[Makoto Kato [:m_kato]]

Attachment: Part 2. Build AVX code on all Windows build

Comment 25

[Ralph Giles (:rillian) | needinfo me]

Comment on attachment 8565275 [details] [diff] [review]
Part 1. Always use VS2013 target

Review of attachment 8565275 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the clean update.

Comment 26

[Ralph Giles (:rillian) | needinfo me]

Comment on attachment 8565305 [details] [diff] [review]
Part 2. Build AVX code on all Windows build

Works for me. Adding ted for build peer review.

Comment 27

[(not currently active) Ted Mielczarek]

Comment on attachment 8565305 [details] [diff] [review]
Part 2. Build AVX code on all Windows build

Review of attachment 8565305 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libvpx/moz.build
@@ +42,5 @@
>          SOURCES += files['X86-64_ASM']
>  
>      # AVX2 only supported on
> +    # Darwin and Windows toolchains right now
> +    if CONFIG['OS_TARGET'] in ('Darwin', 'WINNT'):

You can actually use a set literal for this, not that it makes much difference, that'd be {'Darwin', 'WINNT'}

Comment 28

[Ralph Giles (:rillian) | needinfo me]

To clarify, searching set literals is in general faster than searching tuples, since there's a hash lookup function.

Comment 29

[Mike Hommey [:glandium] (high latency)]

(In reply to Ralph Giles (:rillian) from comment #28)
> To clarify, searching set literals is in general faster than searching
> tuples, since there's a hash lookup function.

That's a very common misconception that should be beaten to death. A hash lookup in a small hash table is generally slower than a linear lookup in a small array. In python, it's an order of magnitude slower.

Comment 30

[Ralph Giles (:rillian) | needinfo me]

(In reply to Mike Hommey [:glandium] from comment #29)

> That's a very common misconception

$ python -m timeit "'hello' in ('hello', 'goodbye')"
10000000 loops, best of 3: 0.035 usec per loop
$ python -m timeit "'hello' in ('goodbye', 'hello')"
10000000 loops, best of 3: 0.045 usec per loop

$ python -m timeit "'hello' in {'hello', 'goodbye'}"
10000000 loops, best of 3: 0.115 usec per loop
$ python -m timeit "'hello' in {'goodbye', 'hello'}"
10000000 loops, best of 3: 0.115 usec per loop

So the tuple is 2-3 times faster in this test. In fact, there seems to be no effective crossover on my system. Worst case with 10000 strings ('benchmark' isn't in /usr/share/doc/words):

$ python -m timeit "'benchmark' in ( $(for word in $(head -10000 /usr/share/dict/words); do echo "'$word', "; done) 'benchmark')"
10000 loops, best of 3: 115 usec per loop
$ python -m timeit "'benchmark' in { $(for word in $(head -10000 /usr/share/dict/words); do echo "'$word', "; done) 'benchmark'}"
1000 loops, best of 3: 418 usec per loop

I guess the overhead of constructing the data structure dominates when there's only a single lookup.

> that should be beaten to death.

Can we discuss best coding practices using less violent metaphors, please?

Comment 31

[Ralph Giles (:rillian) | needinfo me]

Pushed to try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=41c4771b45d5

Comment 32

[Ralph Giles (:rillian) | needinfo me]

Greenish on try.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba871c6324bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d7af888331b

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ba871c6324bf
https://hg.mozilla.org/mozilla-central/rev/8d7af888331b