1536070 - Enable dav1d assembly on BSDs

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Jan Beich]

Since --with-system-dav1d doesn't exist yet disabled assembly makes media.av1.use-dav1d slower than default (libaom). Let's break the platform whitelists by making Linux case generic to any ELF system. Unlike Linux some 32-bit systems (e.g., FreeBSD, OpenBSD) kept 4-byte alignment by default, so explicit -mstack-alignment=16 is required.

$ cc --version
FreeBSD clang version 8.0.0 (branches/release_80 355677) (based on LLVM 8.0.0)
Target: x86_64-unknown-freebsd13.0
Thread model: posix
InstalledDir: /usr/bin

$ cat config.asm
; Autogenerated by the Meson build system.
; Do not edit, your changes will be lost.

%define ARCH_X86_32 0

%define ARCH_X86_64 1

%define STACK_ALIGNMENT 32

vs.

$ cc --version
FreeBSD clang version 8.0.0 (branches/release_80 355677) (based on LLVM 8.0.0)
Target: i386-unknown-freebsd13.0
Thread model: posix
InstalledDir: /usr/bin

$ cat config.asm
; Autogenerated by the Meson build system.
; Do not edit, your changes will be lost.

%define ARCH_X86_32 1

%define ARCH_X86_64 0

%define PIC 1

%define STACK_ALIGNMENT 16

Comment 1

[Alex Chronopoulos [:achronop]]

Thanks for the report, we are about to enable dav1d in Linux.

In Linux we keep stack alignment to 16 due to crashes (Bug 1515933) even on x64. We do rely on the defaults for that. I guess we need to change it now and set the stack alignment flags explicitly.

Comment 2

[Jan Beich]

Attachment: v0

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

Something is weird. I don't notice perf impact unlike with standalone dav1d.

Comment 3

[Alex Chronopoulos [:achronop]]

Have you set "media.av1.use-dav1d" pref to true?

Comment 4

[Jan Beich]

Obviously. Looks like media.rdd-process.enabled (per bug 1534814) is the culprit.

Comment 5

[Jan Beich]

Comment on attachment 9051705 [details] [diff] [review]
v0

Finally managed to confirm it works on FreeBSD i386 on Intel i7-6700k. Like standalone dav1d limits AVX2 to x86-64, so as of dav1d-0.2.1 performance is still inferior to libaom. For example, libaom manages 1440p60 without framedrops but dav1d cannot even 1080p60.

Comment 6

[Jan Beich]

Comment on attachment 9051705 [details] [diff] [review]
v0

(Replace f+/f- with r+/r- here! Not on Phabricator as it requires MFA which is incompatible with GitHub login. Too bad, BMO MFA doesn't support 3FA like stacking 2FA of different providers e.g., GitHub 2FA + VideoLAN GitLab 2FA.)

Comment 7

[Jan Beich]

Comment on attachment 9051705 [details] [diff] [review]
v0

Ah, Alex moved parts of this into bug 1536538. I need to rebase first.

Comment 8

[Jan Beich]

Attachment: v1

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

BMO disabled r?, so assume f? is actually r?.

Comment 9

[Alex Chronopoulos [:achronop]]

Is somewhere documented that default stack alignment is 4 bytes on x86 ? The only doc I can find is the gcc docs that default is -mpreferred-stack-boundary=4 (16-byte alignment), which must set a default -mincoming-stack-boundary=4.

Comment 10

[Jan Beich]

(In reply to Alex Chronopoulos [:achronop] from comment #9)
See https://lists.freebsd.org/pipermail/freebsd-current/2012-November/037563.html
which points to http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38496

Comment 11

[Jan Beich]

Clang has a concise list which OSes changed from 4-byte to 16-byte stack alignment by default:
https://github.com/llvm/llvm-project/blob/llvmorg-8.0.0/llvm/lib/Target/X86/X86Subtarget.cpp#L282-L288
https://github.com/llvm/llvm-project/blob/llvmorg-8.0.0/llvm/lib/Target/X86/X86Subtarget.h#L410-L412

Comment 12

[Alex Chronopoulos [:achronop]]

Thank you for the info. We need to limit that on x86 otherwise I get the following build error:
cc1: error: -mincoming-stack-boundary=2 is not between 3 and 12

Also, in Linux we want to force 16-byte alignment for both x86 and x86_64.

Finally, Bug 1536538 has created, again, the need for rebase but I can help you on that.

Comment 13

[Jan Beich]

(In reply to Alex Chronopoulos [:achronop] from comment #12)

Thank you for the info. We need to limit that on x86 otherwise I get the
following build error:
cc1: error: -mincoming-stack-boundary=2 is not between 3 and 12

Not sure what are you trying to prove. -mincoming-stack-boundary=2 is neither in standalone build nor is needed here.

Also, in Linux we want to force 16-byte alignment for both x86 and x86_64.

How is it related to this bug? I don't see crashes on either i386 (aka x86) or amd64 (aka x86_64).

Finally, Bug 1536538 has created, again, the need for rebase but I can help you on that.

Looks like I need to revert to v0 i.e., pass -mstack-alignment=16 / -mpreferred-stack-boundary=4 on i386 (aka x86) again. On Linux those are nop but elsewhere using 4-byte stack alignment may to lead to crashes as SSE2 assumes 16-byte alignment. And, if not obvious, BSDs don't force SSE2 on i386 (aka x86) by default unlike Linux.

Comment 14

[Jan Beich]

(In reply to Jan Beich from comment #13)

(In reply to Alex Chronopoulos [:achronop] from comment #12)

Thank you for the info. We need to limit that on x86 otherwise I get the
following build error:
cc1: error: -mincoming-stack-boundary=2 is not between 3 and 12

Not sure what are you trying to prove. -mincoming-stack-boundary=2 is
neither in standalone build nor is needed here.

Nevermind. I keep confusing -mincoming-* and -mpreferred-*. Either way my version built fine (see Try in comment 2), so whatever broke build for you (Try links in bug 1536538 don't work for me) was probably obfuscation/optimization for Linux special/broken case.

Comment 15

[Alex Chronopoulos [:achronop]]

(In reply to Jan Beich from comment #13)

Not sure what are you trying to prove. -mincoming-stack-boundary=2 is neither in standalone build nor is needed here.

it is needed when we set the flags for entrypoint_source_files. I said that we need to keep it limited on x86 as you have done in your patches.

How is it related to this bug? I don't see crashes on either i386 (aka x86) or amd64 (aka x86_64).

We don't have crashes because we are on 16-byte stack alignment already in Linux. In comment 1 mentioned that this is due to crashes reported in Bug 1515933.

This is related to this bug because, on Linux, we need to allow the default stack alignment for amd64 but to set the stack alignment explicitly in i386 (previous comment).

Also, in asm/moz.build we need to use the asm/x86_64/linux/config.as for Linux and not the asm/x86_64/config.as.

Looks like I need to revert to v0 i.e., pass -mstack-alignment=16 / -mpreferred-stack-boundary=4 on i386 (aka x86) again. On Linux those are nop but elsewhere using 4-byte stack alignment may to lead to crashes as SSE2 assumes 16-byte alignment. And, if not obvious, BSDs don't force SSE2 on i386 (aka x86) by default unlike Linux.

nice, please note that the change in config.h is already landed.

Comment 16

[Alex Chronopoulos [:achronop]]

(In reply to Jan Beich from comment #14)

so whatever broke build for you (Try links in bug 1536538 don't work for me) was probably obfuscation/optimization for Linux special/broken case.

The try link is the following, it's something broken on our end and reports the wrong links in the bugs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fff724e2b985694dc3724876ceb5aff954ecde27

My build broke because I used the -mpreferred-stack-boundary=4/-mincoming-stack-boundary=2 for the entry source files on amd64.

Comment 17

[Jan Beich]

Attachment: v0.1

(BMO disabled r?, so assume f? is actually r?.)

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

Comment 18

[Jan Beich]

Attachment: Re-enable AV1

A regression from review by a non-build peer.

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

Comment 19

[Alex Chronopoulos [:achronop]]

Do you mind if I do small modifications in your patches and put them up for review? If you create an account in [1] you would be able to post your comments there.

[1] https://phabricator.services.mozilla.com/

Comment 20

[Jan Beich]

(In reply to Alex Chronopoulos [:achronop] from comment #19)

Do you mind if I do small modifications in your patches and put them up for
review?

Sure but I may need to check the code still works on FreeBSD amd64 + i386 and doesn't exclude at least DragonFly, NetBSD, OpenBSD, Solaris.

If you create an account in [1] you would be able to post your comments there.

[1] https://phabricator.services.mozilla.com/

I can't login due to "Bugzilla MFA Not Enabled", see bug 1536716. Posting comments on Phabricator won't be possible.

Comment 21

[Jan Beich]

Sorry for confusing wording. My patches here are in the public domain. Feel free to do whatever.

Comment 22

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1536070 - Enable ASM in dav1d for Tier3 platforms.

Comment 23

[Alex Chronopoulos [:achronop]]

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

Comment 24

[Alex Chronopoulos [:achronop]]

(In reply to Jan Beich from comment #21)

Sorry for confusing wording. My patches here are in the public domain. Feel free to do whatever.

Sure thanks a lot, I believe we can work together to get them landed properly.

I have just posted the patch including your changes with some small modification. I believe we need them in order to avoid crashes. I wont land them until you give me the green light, which means to agree with the code and check that it works in every os/platform you think it should work.

Since phabricator does not work post here any comment or test result.

Comment 25

[Alex Chronopoulos [:achronop]]

The correct try link is: https://treeherder.mozilla.org/#/jobs?repo=try&revision=147dd528ed76c60a12f13c006d31d6cd776a4a47

Comment 26

[Jan Beich]

Comment on attachment 9052582 [details]
Bug 1536070 - Enable ASM in dav1d for Tier3 platforms.

Works fine but I don't like the style.

• Bug 1515933 doesn't affect FreeBSD amd64 (aka x86_64 or x64), so forcing 16-byte (re)alignment with AVX2 may affect performance
• BSDs and Solaris are not "Linux flavors": kernel and libc are different; see bug 1401093 how dangerous such assumptions can be
• media/libdav1d/asm/moz.build and toolkit/moz.configure are out of sync, breaking build on GNU/kFreeBSD

Comment 27

[Jan Beich]

Landry, are you fine with OpenBSD being treated as a "Linux flavor"? If not apply my patches and confirm bundled dav1d doesn't crash on amd64 when playing AV1 content. Note, (on Firefox 67) apply patches in bug 1533559 and bug 1536538 as well.

OTOH, even Linux will be fixed soon or you don't care enough testing AV1 support in advance would be appreciated to avoid surprises like bug 952048.

Comment 28

[Landry Breuil (:gaston)]

I will eventually look at dav1d when its enabled by default for us or starts breaking the build. Those days my time for that is limited, i never really looked at phabricator, i login less and less to bugzilla (because MFA and i dont always have my cell around), and i stopped looking at trunk status on my buildbot (mozbuild is unreliable), i only build released betas. Quickly skimming through alex's patch and yours, i have no opinion which is 'better'.

If you provide me a testcase with a file/url to play, and a clear list of patches to apply on top of a given branch, i can give it a shot, but i can promise nothing, sorry.

Comment 29

[Landry Breuil (:gaston)]

Btw i'd be also interested by --with-system-dav1d but from my limited understanding this wont happen as long as the lib is a moving target.

Comment 30

[Thomas Daede [:TD-Linux]]

(In reply to Jan Beich from comment #26)

• Bug 1515933 doesn't affect FreeBSD amd64 (aka x86_64 or x64), so forcing 16-byte (re)alignment with AVX2 may affect performance

Both Linux and FreeBSD use the same ABI here, so the LTO bug will affect both.

Comment 31

[Jan Beich]

Alex, can you adjust STACK_ALIGNMENT in media/libdav1d/config.h as well?

(In reply to Thomas Daede [:TD-Linux] from comment #30)

Both Linux and FreeBSD use the same ABI here, so the LTO bug will affect both.

Neither bug 1515933 nor upstream mentioned LTO. After testing I can confirm LTO on FreeBSD is also affected.

Comment 32

[Alex Chronopoulos [:achronop]]

I have pushed in phabricator an update if you want to have a look.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc78d8cad2ce
Enable ASM in dav1d for Tier3 platforms. r=TD-Linux

Comment 34

[Jan Beich]

Comment on attachment 9052582 [details]
Bug 1536070 - Enable ASM in dav1d for Tier3 platforms.

I confirm, this version (i.e., what landed) works fine on FreeBSD amd64 + i386.

Comment 35

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/cc78d8cad2ce

Comment 36

[Jan Beich]

LTO crash also affects i386 (aka x86) on BSDs because of 4-byte default. Like amd64 GCC LTO isn't affected.

Comment 37

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/projects/ash/rev/cc78d8cad2cedab669fddcc804d6282a2af900a2
Bug 1536070 - Enable ASM in dav1d for Tier3 platforms. r=TD-Linux

Comment 38

[Alex Chronopoulos [:achronop]]

Comment on attachment 9052582 [details]
Bug 1536070 - Enable ASM in dav1d for Tier3 platforms.

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: dav1d support Linux BSD. We need it in order to enable dav1d on beta.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1535631
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Dav1d on Linux is stable, this patch adds more Linux flavors.
• String changes made/needed:

Comment 39

[Landry Breuil (:gaston)]

(In reply to Alex Chronopoulos [:achronop] from comment #38)

• Why is the change risky/not risky? (and alternatives if risky): Dav1d on Linux is stable, this patch adds more Linux flavors.

Can you avoid referring to other non-linux non-windows non-macos non-android operating systems as "Linux flavors" ? I personally don't really care but some ppl are sensitive about that. Thanks!

Comment 40

[Pascal Chevrel:pascalc (PTO until April 26)]

(In reply to Alex Chronopoulos [:achronop] from comment #38)

Comment on attachment 9052582 [details]
Bug 1536070 - Enable ASM in dav1d for Tier3 platforms.

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: dav1d support Linux BSD. We need it in order to enable dav1d on beta.

Wait, we need to enable BSD support on beta so as to also enable dav1d support on our tier1 platforms on beta? Alex can you elaborate on this please? Thanks

Comment 41

[Alex Chronopoulos [:achronop]]

(In reply to Pascal Chevrel:pascalc from comment #40)

Wait, we need to enable BSD support on beta so as to also enable dav1d support on our tier1 platforms on beta? Alex can you elaborate on this please? Thanks

This prevents a problem in the compiler and can affect Linux and FreeBDS. If we don't care about building beta with any compiler we can avoid it. However, I would prefer to take it because the following patches have been based on it and it is safer to maintain the same code level between beta and nightly. Also, it's a small patch of low risk.

Comment 42

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9052582 [details]
Bug 1536070 - Enable ASM in dav1d for Tier3 platforms.

Patch needed for the libdav1d update to 0.2.1 on beta. Uplift accepted for 67 beta 8, thanks.

Comment 43

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/039c28632803