1248020 - Add something to moz.build to use yasm as $(AS)

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

Right now we have a few places in the tree that want to use yasm as their assembler, but they have to set AS = $(SOMETHING) in their Makefile.in, like:
https://dxr.mozilla.org/mozilla-central/rev/576a6dcde5b68c2ea45324ed5ce1dabb7d833d09/media/ffvpx/libavcodec/x86/Makefile.in#6

We should add something like `USE_YASM = True` to moz.build which would force `AS := $(YASM)`. We could get rid of all those `FOO_AS` variables once we do that.

(bug 1248016 covers the other line in that Makefile, ASM_SUFFIX.)

Comment 1

[(not currently active) Ted Mielczarek]

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

Comment 2

[(not currently active) Ted Mielczarek]

Attachment: MozReview Request: bug 1248020 - add USE_YASM to moz.build. r?mshal

Review commit: https://reviewboard.mozilla.org/r/37917/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37917/

Comment 3

[(not currently active) Ted Mielczarek]

Attachment: MozReview Request: bug 1248020 - convert a bunch of moz.build files to use USE_YASM. r?mshal

Review commit: https://reviewboard.mozilla.org/r/37919/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37919/

Comment 4

[(not currently active) Ted Mielczarek]

So these patches got a little complicated. The first one is fairly straightforward--it adds `USE_YASM` to the moz.build context, which just sets a few passthrough variables to do what the existing Makefiles do--set AS:=$(YASM) and ASFLAGS:=$(YASM_ASFLAGS). It does copy a bit of logic from elsewhere in configure to get a usable set of default flags for yasm.

The second patch is more complicated because the configure logic for the things using yasm is complicated! First of all, we don't require yasm, and we don't use it on every platform (It only supports x86 and x86-64, for one thing), so we still have to keep some logic for falling back to the default assembler for some things. I tried to simplify things a little bit--nothing AC_SUBSTS a `FOO_AS` value anymore that points to yasm, they just either have a flag indicating they should use yasm (VPX, LIBJPEG_TURBO), or they only support yasm and nothing else (LIBAV_FFT, FFVPX). Each set of ASFLAGS also got pruned down to just the set of things that aren't in YASM_ASFLAGS. These could probably even just be moved into moz.build at this point, but I didn't want to make this patch even more complicated.

The config.mk changes are necessary because the ASFLAGS for yasm are getting set in backend.mk now, which comes before rules.mk and so that block was resetting them, which was no good. The AS_DASH_C bit is sent down as a passthrough variable in emitter.py, so that's not necessary either. The rest of the patch is just moz.build fiddling to add USE_YASM where needed and remove the no-longer-necessary Makefile.in files.

Comment 5

[Michael Shal [:mshal]]

Comment on attachment 8726295 [details]
MozReview Request: bug 1248020 - add USE_YASM to moz.build. r?mshal

https://reviewboard.mozilla.org/r/37917/#review34575

::: old-configure.in:3410
(Diff revision 1)
> +      YASM_ASFLAGS="-f x64 -rnasm -pnasm"

From 'yasm -f help', it looks like 'win64' should also work for x64, and might make it a little more clear here. I see where this came from now in the second patch, so consider it a nit if you don't want to change the behavior :)

::: old-configure.in:3416
(Diff revision 1)
> +      if $CC -E -dM -</dev/null | grep -q __ELF__; then

If this check is false, do we need to unset YASM rather than just leaving YASM_ASFLAGS blank?

::: old-configure.in:3421
(Diff revision 1)
> +      if $CC -E -dM -</dev/null | grep -q __ELF__; then

Similar to above.

Comment 6

[Michael Shal [:mshal]]

https://reviewboard.mozilla.org/r/37917/#review34581

::: old-configure.in:3410
(Diff revision 1)
> +      YASM_ASFLAGS="-f x64 -rnasm -pnasm"

On third thought, LIBAV_FFT_ASFLAGS used to use '-f win64', so maybe we should go with that to be a little more readable.

Comment 7

[Michael Shal [:mshal]]

Comment on attachment 8726296 [details]
MozReview Request: bug 1248020 - convert a bunch of moz.build files to use USE_YASM. r?mshal

https://reviewboard.mozilla.org/r/37919/#review34579

I think all of the moz.build / configure changes look good. Is there a way you can double-check to make sure we didn't accidentally stop using asm where we should? Maybe compare the number of 'yasm' invocations in the before & after on each platform would suffice. I'd hate to lose performance over a build config change.

::: old-configure.in:5467
(Diff revision 1)
> +      VPX_ASFLAGS="-DPIC"

I think you are now adding -DPIC where it wasn't used before (WINNT:x86_64 and *:arm*). Does that matter?

Comment 8

[(not currently active) Ted Mielczarek]

https://reviewboard.mozilla.org/r/37919/#review35129

::: old-configure.in:5467
(Diff revision 1)
> +      VPX_ASFLAGS="-DPIC"

Good catch! I think this is only the Win64 case, the arm case doesn't use yasm. I'll fix it to match what we currently have. I don't know if it matters, but I'd rather not change it.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 8726295 [details]
MozReview Request: bug 1248020 - add USE_YASM to moz.build. r?mshal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37917/diff/1-2/

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 8726296 [details]
MozReview Request: bug 1248020 - convert a bunch of moz.build files to use USE_YASM. r?mshal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37919/diff/1-2/

Comment 11

[Michael Shal [:mshal]]

Comment on attachment 8726295 [details]
MozReview Request: bug 1248020 - add USE_YASM to moz.build. r?mshal

https://reviewboard.mozilla.org/r/37917/#review35135

::: old-configure.in:3430
(Diff revisions 1 - 2)
> +    # If we didn't get ASFLAGS, we shouldn't be using yasm.

nit: mind as well say YASM_ASFLAGS in the comment instead of ASFLAGS to line up exactly with the variable name.

Comment 12

[Michael Shal [:mshal]]

Comment on attachment 8726296 [details]
MozReview Request: bug 1248020 - convert a bunch of moz.build files to use USE_YASM. r?mshal

https://reviewboard.mozilla.org/r/37919/#review35137

Comment 13

[(not currently active) Ted Mielczarek]

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

Comment 14

[(not currently active) Ted Mielczarek]

https://reviewboard.mozilla.org/r/37917/#review34581

> On third thought, LIBAV_FFT_ASFLAGS used to use '-f win64', so maybe we should go with that to be a little more readable.

This burned on try:
https://treeherder.mozilla.org/logviewer.html#?job_id=17757164&repo=try

presumably because this check is looking for exactly win64:
https://dxr.mozilla.org/mozilla-central/rev/05c087337043dd8e71cc27bdb5b9d55fd00aaa26/media/libvpx/vpx_config.asm#11

I don't know why yasm would implement two options for the same format but not expose them as the same thing internally, but I'm going to leave this as x64 because I don't want to touch any of the assembly code.

Comment 15

[(not currently active) Ted Mielczarek]

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

Comment 16

[(not currently active) Ted Mielczarek]

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

Comment 17

[(not currently active) Ted Mielczarek]

Actual successful Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e94afcfb3333

I downloaded all the logs and compared vs this try run of a stock m-c changeset:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4f33b054986&selectedJob=17757664

I did a quick sanity check "grep 'yasm -o' | wc -l" to make sure the number of yasm invocations was the same on both pushes on all platforms. Then I did some spot-checks to check that the yasm commandlines were the same. They were fine on everything *but* Win64. As it turns out, *some* of the Makefiles using yasm used '-f win64' where others used '-f x64'.

All the other asm files that check the output format seem to handle win64/x64 interchangably though:
https://dxr.mozilla.org/mozilla-central/rev/f0c0480732d36153e8839c7f17394d45f679f87d/media/libvpx/third_party/x86inc/x86inc.asm#51

So I think this should be fine.

Comment 18

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8278f56b336fd0d859db8cabdce102f63f7a3b3
bug 1248020 - add USE_YASM to moz.build. r=mshal

https://hg.mozilla.org/integration/mozilla-inbound/rev/116882b45609d0ceac5cb80fe71c04889e6ec187
bug 1248020 - convert a bunch of moz.build files to use USE_YASM. r=mshal

Comment 19

[(not currently active) Ted Mielczarek]

rillian: FYI, I reworked the way various media bits invoke yasm in this patch. I verified that the yasm commandlines are the same (modulo the bit in comment 17 about Win64).

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f8278f56b336
https://hg.mozilla.org/mozilla-central/rev/116882b45609