Closed Bug 1540124 Opened 6 years ago Closed 6 years ago

Enable dav1d assembly builds in arm64

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
66 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox68 --- fixed

People

(Reporter: achronop, Assigned: achronop)

References

Details

Attachments

(1 file)

Bug 1540124 - Enable aarch64 assembly builds for libdav1d. r?TD-Linux
47 bytes, text/x-phabricator-request
Details | Review
No description provided.

It's not a rush but I gave it a try this morning, I am getting a build error about a bad argument in nasm [1]:

12:23:50 INFO - z:/build/build/src/nasm/nasm.exe -o cdef.obj -DNDEBUG=1 -DTRIMMED=1 -DDAV1D_API= -DSTACK_ALIGNMENT=16 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -F cv8 -Iz:/build/build/src/third_party/dav1d -Iz:/build/build/src/third_party/dav1d/include -Iz:/build/build/src/third_party/dav1d/src -Iz:/build/build/src/third_party/dav1d/src/ -c z:/build/build/src/media/libdav1d/asm/../../../third_party/dav1d/src/arm/64/cdef.S
12:23:50 INFO - nasm: error: unrecognised option -c' 12:23:50 INFO - typenasm -h' for help
12:23:50 INFO - z:/build/build/src/config/rules.mk:801: recipe for target 'cdef.obj' failed
12:23:50 INFO - mozmake.EXE[4]: *** [cdef.obj] Error 1

This was windows but I am getting the same error in a Linux local build.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a84230857a8fa6f598f6461fdd64f0e9c3d1794

Depends on: 1540369

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

Try run on the top of 1540369:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9446beba028a1f7964f7e0e7728de8224dcf379

It still looks like it is calling the wrong native assembler there.

:glandium, do we need to set AS explicitly in the build configs for AArch64?

Flags: needinfo?(mh+mozilla)

Seems like you want to not unconditionally set USE_NASM, but only set it for architectures where nasm can be used:

https://hg.mozilla.org/try/rev/26872b342e3829551459899367469f35f4deb591#l1.76

On the patch in the try run [1] above this is not set unconditionally. Is only set in x86.

[1] https://hg.mozilla.org/try/rev/bc49d1b647620be5cdfd6fe4a9bc1362a2f09e34

(In reply to Nathan Froyd [:froydnj] from comment #4)

Seems like you want to not unconditionally set USE_NASM, but only set it for architectures where nasm can be used:

https://hg.mozilla.org/try/rev/26872b342e3829551459899367469f35f4deb591#l1.76

The try run from comment 2 already has that fixed.

The problem right now is that the new AS is not used. I've made another try push here:

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

where I set AS to the new target-specific assembler:

https://hg.mozilla.org/try/rev/15461a18a2fb4ce7cfcd4bafa413850fa6538a8c#l1.12

But apparently we can't just do that globally, though I wonder what ICU is building there exactly.

So the problem here actually simply is that the target-specific AS isn't on PATH and apparently Clang just picks the first it finds. This try push is green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5a3b665775c96b14e2af93d7d1956f785156b7f&selectedJob=237361830

Now that is of course not the right fix, but it shows that putting the new AS on PATH works:

https://hg.mozilla.org/try/diff/2d75c2c63e43/build/unix/mozconfig.unix#l1.13

(In reply to Christian Holler (:decoder) from comment #6)

But apparently we can't just do that globally, though I wonder what ICU is building there exactly.

When I looked at it (to use nasm globally on x86_64) it was just a bunch of static tables. They can also be made by generating C code, presumably it's faster to compile the assembly (though probably not by much nowadays).

Depends on: 1540882

With the patch in bug 1540882, I got the try from comment 5 building.

Flags: needinfo?(mh+mozilla)

This is the latest run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcda1bbc04b73b33fab67f4b9dd6c9ace867c9f0

Linux is green.
Windows fails with error:

10:35:51     INFO -  z:/build/build/src/vs2017_15.9.6/VC/bin/HostX64/arm64/armasm64.exe -o cdef.obj -DNDEBUG=1 -DTRIMMED=1 -DDAV1D_API= -DSTACK_ALIGNMENT=16 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -Iz:/build/build/src/third_party/dav1d -Iz:/build/build/src/third_party/dav1d/include -Iz:/build/build/src/third_party/dav1d/src -Iz:/build/build/src/third_party/dav1d/src/ -Iz:/build/build/src/obj-firefox/dist/include/dav1d/  -c z:/build/build/src/media/libdav1d/asm/../../../third_party/dav1d/src/arm/64/cdef.S
10:35:51     INFO -  Microsoft (R) ARM Macro Assembler Version 14.16.27026.1 for 64 bits
10:35:51     INFO -  Copyright (C) Microsoft Corporation.  All rights reserved.
10:35:51     INFO -  error A2029: unknown command-line argument or argument value -DNDEBUG=1
10:35:51     INFO -   Usage:      armasm [<options>] sourcefile objectfile
10:35:51     INFO -               armasm [<options>] -o objectfile sourcefile
10:35:51     INFO -               armasm -h              for help
10:35:51     INFO -  z:/build/build/src/config/rules.mk:801: recipe for target 'cdef.obj' failed
10:35:51     INFO -  mozmake.EXE[4]: *** [cdef.obj] Error 1

I have been told in #dav1d that in order to use armasm64, we would need to use gas-preprocessor [1] to reformat the source from gas format to armasm format. Also, that with *.S files a standalone gas is not needed. Those files it's fine to be passed directly to the compiler. In the matter of fact this is needed because they are not plain assembly files (like *.s or *.asm) and they need to be run via the preprocessor.

On a parallel note, we tried to pass those files directly to the compiler when we were working on Linux case and it didn't work.

[1] https://git.libav.org/?p=gas-preprocessor.git;a=summary

We already have two other variants of that script in our repo (ads2gas.pl in libvpx and arm2gnu.pl in libtheora), they both have custom Makefile.in rules to call them. We just need to copy in gas-preprocessor and put in the same rules for libdav1d.

See Also: → 1540760

I am depending this one on Dan's work because he adds there the gas-preprocessor.pl which will be used here as well.

Depends on: 1540760

I was totally mixed up, gas-preprocessor.pl is the exact opposite of the other two. The dav1d .S files are already gas syntax and can be shoved directly into clang, you only need to use it if you want to use armasm.

In the following run [1] the build files are configured to use clang for the assembly files (*.S), in conjunction with -integrated-as (which is the default). This works well on Linux, clang builds the assembly files successfully, and the build is green. On the other hand windows does not behave the same way. The assembler, armasm64, is invoked to build the assembly files, even though integrated as has been requested, and the build fails.

For the history, it is possible to work around the windows error by using the same steps described in Bug 1540760 which are to parse the *.S file with gas-preprocessor and change the file extension from .S to .asm [2].

Nevertheless if it was possible to teach our windows build system to compile the *.S files with clang instead of armasm, no modification would be necessary. The Linux example shows that clang can handle it successfully. In addition to that, since integrated as has been requested, invoking an external assembler looks like a bug in our build system.

Nathan, do you think it is possible to use clang to compile the *.S files for the windows builds? Is there a quick way to try that out?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=239732658&revision=d13f304f676101899cffe7b5dae2bcc49b6e76a7
[2] https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=239732658&revision=3d6afedc77e0be74457200cb457266f0d6d02a7c

Flags: needinfo?(nfroyd)

I created a run [1] that uses clang-cl for assembler. There are build error for *.asm files which is expected. I am looking for a way to pass the *.S files to clang-cl and not to change the assembler in total.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c92cffe5ddaad269b850bd9e7365c90ef18fc7e

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

In the following run [1] the build files are configured to use clang for the assembly files (*.S), in conjunction with -integrated-as (which is the default). This works well on Linux, clang builds the assembly files successfully, and the build is green. On the other hand windows does not behave the same way. The assembler, armasm64, is invoked to build the assembly files, even though integrated as has been requested, and the build fails.

-integrated-as is an option passed to the compiler, not the build system. The build system uses AS for compiling assembly files (.asm and .S). AS and its associated options and info is all set up here:

https://searchfox.org/mozilla-central/source/build/moz.configure/toolchain.configure#2056-2156

You'll see that for clang-cl, we use some external assembler always:

https://searchfox.org/mozilla-central/source/build/moz.configure/toolchain.configure#2061-2077

For the history, it is possible to work around the windows error by using the same steps described in Bug 1540760 which are to parse the *.S file with gas-preprocessor and change the file extension from .S to .asm [2].

The generated .asm has the appropriate syntax for armasm64, so this works.

Nevertheless if it was possible to teach our windows build system to compile the *.S files with clang instead of armasm, no modification would be necessary. The Linux example shows that clang can handle it successfully. In addition to that, since integrated as has been requested, invoking an external assembler looks like a bug in our build system.

This is not a bug, for the aforementioned reason that the integrated assembler is a compiler option, not a build system option.

Nathan, do you think it is possible to use clang to compile the *.S files for the windows builds? Is there a quick way to try that out?

It's possible? You could try making the rule for SOBJS:

https://searchfox.org/mozilla-central/source/config/rules.mk#800

use $(CC) instead of $(AS), and maybe massaging it to look more like the $(ASOBJS) rule, a couple of lines above. It's also possibly we could add a no_really_use_the_compiler flag for assembly sources. Not sure how much would break...Is this really more useful than just using gas-preprocessor?

It looks like clang now has a fully-featured gas compatible assembly parser, which it didn't have back in the day when we added -fno-integrated-as or whatever. It's possible that some of those uses around the tree could go away, but that would be subject to whether our minimum supported clang does the gas thing, too.

Flags: needinfo?(nfroyd)

(In reply to Nathan Froyd [:froydnj] from comment #16)

It's possible? You could try making the rule for SOBJS:

https://searchfox.org/mozilla-central/source/config/rules.mk#800

use $(CC) instead of $(AS), and maybe massaging it to look more like the $(ASOBJS) rule, a couple of lines above. It's also possibly we could add a no_really_use_the_compiler flag for assembly sources. Not sure how much would break...

Thanks a lot for the above. That gives me a green try [1]. I am sure it's not the proper fix but it was enough to try it out.

Is this really more useful than just using gas-preprocessor?

Well it's a little more convenient because it does not require any preprocessing of the initial files. The other way requires to land a modified version of gas-preprocessor, parse the files manually and add the new files on the tree. However, it's not a big issue we can go on with gas-preprocessor. In general, it was nice to know that it could work that way too. We may want to update it in the future.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=00c1379e121b3b0e6976721cb5bf3b81bd89fadf

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

(In reply to Nathan Froyd [:froydnj] from comment #16)

Is this really more useful than just using gas-preprocessor?

Well it's a little more convenient because it does not require any preprocessing of the initial files.

Why not run the preprocessor at build time, on the original files, every build? That is, make the build use GENERATED_FILES. I guess the preprocessor is written in perl, which we technically don't require, but I think we do still have some perl scripts in the tree somewhere...

(In reply to Nathan Froyd [:froydnj] from comment #18)

Why not run the preprocessor at build time, on the original files, every build? That is, make the build use GENERATED_FILES. I guess the preprocessor is written in perl, which we technically don't require, but I think we do still have some perl scripts in the tree somewhere...

This would add some unnecessary complexity without a real benefit. Those files does not change often so we don't need to parse them every time. We only need to parse them once after a new import from upstream. Maybe we can run the preprocessor when we pull from upstream as part of mach vendor dav1d. That also eliminates the perl requirement in build system.

This is a green try using the new flag Dan added in Bug 1540760. The *.S are built with clang-cl for windows due to that flag.

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

Dan, can you please help me to test the aarch64 binaries, I don't have the hardware. The try run is in [1]. You can try the platform you have. A list of AV1 videos is in [2].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8bd694935ad526b2430d6c1ea26ad3829d4d34d
[2] http://download.opencontent.netflix.com/?prefix=AV1/Chimera/

Thanks!

Flags: needinfo?(dminor)

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

Dan, can you please help me to test the aarch64 binaries, I don't have the hardware. The try run is in [1]. You can try the platform you have. A list of AV1 videos is in [2].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8bd694935ad526b2430d6c1ea26ad3829d4d34d
[2] http://download.opencontent.netflix.com/?prefix=AV1/Chimera/

I tested this one (I found a target.installer.exe and ran it), and it seems to run fine, and uses significantly less CPU when decoding the Chimera sample than before (with av1 and dav1d enabled in about:config).

That's perfect thank you very much!

Thanks Martin!

Flags: needinfo?(dminor)
Pushed by achronopoulos@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c49518b4191c Enable aarch64 assembly builds for libdav1d. r=dminor

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

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Assignee: nobody → achronop
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: