1469027 - baldrdash: running wasm with Cranelift

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement)

Description

[Dan Gohman [:sunfish]]

Attachment: preliminary baldrdash integration patch

Attached is the current "baldrdash" patch, which adds a nightly-only option for running wasm with Cretonne. Cretonne is in development and doesn't yet have all its planned optimization features, however it is able to pass the testsuite on x64. The patch has the following known issues:

 - Currently JS shell builds require the environment variable MOZ_CLANG_PATH to be set to a clang binary. This appears to be needed by the bindgen infrastructure. Is there a better way to handle this?

 - The JS shell builds, but full firefox builds currently fail with "mozbuild.configure.ConfigureError: Cannot include `[...]/build/moz.configure/rust.configure` because it was included already.". It appears rust.configure is being included by both toolkit/moz.configure and js/moz.configure. Is there a way to make it conditional on whether the JS shell is being built?

 - Cretonne depends on the failure crate, which isn't yet in firefox. Is this a problem? If so, it'd be possible to remove this dependency or make it optional.

 - wasmparser.rs currently includes 7.4M of test binaries that will get copied into the tree by the vendoring script. Is that too much?

 - Cretonne depends on target-lexicon (which I also maintain), which is licensed as "Apache-2.0 WITH LLVM-exception". This is a SPDX license, however crates.io is still using an older SPDX, so target-lexicon currently uses a license-file. I added it to the whitelist in python/mozbuild/mozbuild/vendor_rust.py, however this is just a temporary workaround.

Comment 1

[Till Schneidereit [:till]]

(In reply to Dan Gohman [:sunfish] from comment #0)
>  - Cretonne depends on target-lexicon (which I also maintain), which is
> licensed as "Apache-2.0 WITH LLVM-exception". This is a SPDX license,
> however crates.io is still using an older SPDX, so target-lexicon currently
> uses a license-file. I added it to the whitelist in
> python/mozbuild/mozbuild/vendor_rust.py, however this is just a temporary
> workaround.

Is this tracked in a GitHub issue? It seems easy enough to do to have it block this. The alternative would probably be to reach out to licensing people (i.e. mhoye), which might be more effort. (OTOH, I don't know if reaching out to the licensing people would still be needed.)

Comment 2

[Dan Gohman [:sunfish]]

(In reply to Till Schneidereit [:till] from comment #1)
> Is this tracked in a GitHub issue? It seems easy enough to do to have it
> block this. The alternative would probably be to reach out to licensing
> people (i.e. mhoye), which might be more effort. (OTOH, I don't know if
> reaching out to the licensing people would still be needed.)

It's being tracked here:

https://github.com/rust-lang-nursery/license-exprs/issues/12

Comment 3

[Jeff Walden [:Waldo]]

(In reply to Dan Gohman [:sunfish] from comment #0)
> Attached is the current "baldrdash" patch, which adds a nightly-only option
> for running wasm with Cretonne.

A+ name

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Dan Gohman [:sunfish] from comment #0)
>  - Currently JS shell builds require the environment variable MOZ_CLANG_PATH
> to be set to a clang binary. This appears to be needed by the bindgen
> infrastructure. Is there a better way to handle this?

We can move the logic for determining where bindgen bits live:

https://searchfox.org/mozilla-central/source/toolkit/moz.configure#666-809

somewhere where JS and Gecko can see it.

>  - The JS shell builds, but full firefox builds currently fail with
> "mozbuild.configure.ConfigureError: Cannot include
> `[...]/build/moz.configure/rust.configure` because it was included
> already.". It appears rust.configure is being included by both
> toolkit/moz.configure and js/moz.configure. Is there a way to make it
> conditional on whether the JS shell is being built?

Can you have JS-only builds without building the shell?  If so, we have a couple of different cases:

- Gecko builds, Rust required
- JS-only builds, shell built, Rust required
- JS-only builds, shell not built, Rust...not required (yet?)?

I think this is doable, but there will be some configure bits to sort out.  I'm assuming we'd like to keep whatever Rust bits the JS engine requires Nightly-only at this point?  Alternatively, do we want to take a hard dependency on Rust in the JS engine now, or wait until ESR 69/70?

>  - Cretonne depends on the failure crate, which isn't yet in firefox. Is
> this a problem? If so, it'd be possible to remove this dependency or make it
> optional.

Assuming the failure crate is reasonable (no huge files, no obvious badness), I don't see why this would be a problem.

>  - wasmparser.rs currently includes 7.4M of test binaries that will get
> copied into the tree by the vendoring script. Is that too much?

I think so.  Is it possible to separate those out into a test-only crate?  I'm not sure if cargo-vendor will pull those in or not...

>  - Cretonne depends on target-lexicon (which I also maintain), which is
> licensed as "Apache-2.0 WITH LLVM-exception". This is a SPDX license,
> however crates.io is still using an older SPDX, so target-lexicon currently
> uses a license-file. I added it to the whitelist in
> python/mozbuild/mozbuild/vendor_rust.py, however this is just a temporary
> workaround.

You can ask for license review from mhoye and we can add that as a permanent exception.

I assume we'll also want some sort of configure flag --enable-cretonne or somesuch, defaulted to on in Nightly and off everywhere else, rather than the hardcoded defines the patch currently has.

Comment 5

[Dan Gohman [:sunfish]]

(In reply to Nathan Froyd [:froydnj] from comment #4)
> (In reply to Dan Gohman [:sunfish] from comment #0)
> >  - wasmparser.rs currently includes 7.4M of test binaries that will get
> > copied into the tree by the vendoring script. Is that too much?
> 
> I think so.  Is it possible to separate those out into a test-only crate? 
> I'm not sure if cargo-vendor will pull those in or not...

I've now filed https://github.com/yurydelendik/wasmparser.rs/issues/55 about moving the tests out.

> >  - Cretonne depends on target-lexicon (which I also maintain), which is
> > licensed as "Apache-2.0 WITH LLVM-exception". This is a SPDX license,
> > however crates.io is still using an older SPDX, so target-lexicon currently
> > uses a license-file. I added it to the whitelist in
> > python/mozbuild/mozbuild/vendor_rust.py, however this is just a temporary
> > workaround.
> 
> You can ask for license review from mhoye and we can add that as a permanent
> exception.

I've now filed bug 1476388 to request a license review.

Comment 6

[Dan Gohman [:sunfish]]

Attachment: baldrdash.patch

Here's an updated patch. Major changes:

 - Update to the latest Cretonne/Cranelift
 - Merge with trunk, and implement realm switching
 - Add the "Apache-2.0 WITH LLVM-exception" license to the whitelist, now that bug 1476388 is resolved.

Also note that in this patch, as well as the previous one, I've excluded the Cargo.lock and third_part/rust changes from this patch, as they're mechanically generated from "mach vendor rust".

Comment 7

[Dan Gohman [:sunfish]]

Attachment: baldrdash.patch

This version adds Cranelift's license to the about:license page, as requested in bug 1476388.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: baldrdash.patch

(rebased and updated to use cranelift 0.17.0)

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: baldrdash.patch

(rebased, without Cargo.lock changes this time)

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2-valtype.patch

More changes since the ValType refactoring.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

As agreed on IRC with Dan, I'll take over this bug to get it landed. I'm currently rebasing this patch, updating Cranelift and adapting the new APIs, while ensuring tests still pass.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: wip.patch

Rolled up wip.

- updated to cranelift 0.20;
- tried using bindgen for the ModuleEnvironment; failed b/o mozilla::Vector and other data structures in there (which use templates etc., which bindgen doesn't seem to support out of the box);
- started to remove all the calls between Spidermonkey and Cranelift; the code and metadata is now stored in a Rust structure that's passed out to C++ afterwards, so code generation entirely happens in C++. (no more need for the Emitter structure either) Did remove a lot of bindings.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: rolledup.patch

Rolled up patch (applies cleanly on top of mozilla-inbound 345269b39c6c), with vendoring etc. Tried to simplify a few things, attempting some different designs, etc.

There are cranelift register allocation failures when trying this patch on AngryBots/Tanks, but I'd like to make sure I didn't introduce them first.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.cranelift-nightly.patch

We'd like to always include Cranelift in Nightly builds, or on demand for non-Nightly. This will increase the binary's size, compilation time (both Spidermonkey and full Gecko); I haven't measured by how much though.

Nathan, does it make sense? Is it the right way to do this with the configure machinery?

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.--wasm-force-cranelift.patch

Adds a --wasm-force-cranelift shell flag, which does nothing at the moment but to set the option in the JS options struct.

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3.tier::cranelift.patch

Introduces the Tier::Cranelift, which can be used as a second tier and binds it to the wasm-force-cranelift shell flag. With these patches, it will just compile with IonMonkey; later patches will make use of Cranelift is Tier is Tier::Cranelift.

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4.bindgen-d-types.patch

This is a preparatory patch for the next one, mostly code motion. It introduces a new header called WasmBasicTypes.h, containing types that are then included in WasmTypes.h or bindgen'd for use in Baldrdash.

The changes are the following:
- code motion (for some types)
- the FuncWithTypeId::Kind is extracted to its own enum class
- a new CPU feature detection of SSE4.2 is added to the Assembler class.

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 5a-cranelift-integration.patch

This is part of the main patch (the rest is automatic generated output, license updates and moz.build goo).

Lars, flagged you for feedback since you wrote this code in the first place. If you'd like to review it too, feel free to do so; the more eyes on this big patch, the better.

This builds on all platforms, and passes tests with --wasm-force-cranelift on x64. On other platforms, since SSE2 is marked as not present (see #ifdef in CraneliftStaticEnvironment ctor), the cranelift_compiler_create function call will fail in make_isa(); so ARM / x86 / others will gracefully fail with a Rust error message ("SSE2 is not present"), reported as an OOM (better error reporting will come in subsequent bugs).

My plan is to try to land this as soon as possible, then have many other bugs to make the integration better/faster, rename things, harmonize names/casing in FFIs, etc.

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 5b-build-support.patch

All the build bits, mostly copied from what was in previous patches.

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 5c-mach-vendor-rust.patch

The output of ./mach vendor rust.

Nathan, I assume this needs to be reviewed/approved? Is there a documented process, if approval is needed?

Comment 21

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 5d-license-changes.patch

Mark, this patch contains the license changes that have been discussed in bug 1476388 and bug 1476798. This will be folded with the rest of the patches numbered 5, which introduce the code the new license applies to.

Comment 22

[Nathan Froyd [:froydnj]]

Comment on attachment 9007273 [details] [diff] [review]
1.cranelift-nightly.patch

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

I believe this is correct.

Comment 23

[Mike Hoye [:mhoye]]

Comment on attachment 9007294 [details] [diff] [review]
5d-license-changes.patch

R+, thank you.

Comment 24

[Nathan Froyd [:froydnj]]

Comment on attachment 9007290 [details] [diff] [review]
5b-build-support.patch

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

r=me with the RUST_STATIC_LIB_FOR_SHARED_LIB change mentioned below, and assuming there's a reasonable answer to the necessity of the .hgignore change.

::: .hgignore
@@ +43,5 @@
>  _DBG\.OBJ/
>  _OPT\.OBJ/
>  ^js/src/.*-obj/
>  ^js/src/obj-.*/
> +^js/src/wasm/cranelift/target

Please add this to .gitignore as well.

But wait, why is this necessary?  Shouldn't the build be putting the output files in the objdir, not the srcdir?  Or is this an attempt to ease local development for people who just want to `cargo build` cranelift and nothing else?

::: config/rules.mk
@@ +552,5 @@
>  #
>  # PROGRAM = Foo
>  # creates OBJS, links with LIBS to create Foo
>  #
> +$(PROGRAM): $(PROGOBJS) $(RUST_STATIC_LIB_FOR_SHARED_LIB) $(STATIC_LIBS) $(EXTRA_DEPS) $(RESFILE) $(GLOBAL_DEPS) $(call mkdir_deps,$(FINAL_TARGET))

I think it would also be a good idea to rename RUST_STATIC_LIB_FOR_SHARED_LIB at this point, since we're now using Rust static libraries in other places than shared libraries. :)

::: js/src/gdb/moz.build
@@ +46,5 @@
> +
> +# The Rust standard library references libresolv on macOS, so we need to link
> +# it as a workaround. See also bug 1367932.
> +if CONFIG['OS_ARCH'] == 'Darwin':
> +    OS_LIBS += ['-lresolv']

I don't think you have to fix this here, but maybe we should just start linking in dependencies for Rust libraries in the build system itself, rather than scattering this stanza around in a bunch of different places.  Please file a followup bug?

Comment 25

[Nathan Froyd [:froydnj]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #20)
> Created attachment 9007292 [details] [diff] [review]
> 5c-mach-vendor-rust.patch
> 
> The output of ./mach vendor rust.
> 
> Nathan, I assume this needs to be reviewed/approved? Is there a documented
> process, if approval is needed?

It does need to be approved, by me or by someone who has experience with the crates being modified.  AFAICT, this is just trivially updating `log` and then adding a bunch of cranelift packages.  Assuming that doesn't change, r=me.

Comment 26

[Luke Wagner [:luke]]

Comment on attachment 9007275 [details] [diff] [review]
3.tier::cranelift.patch

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

Question: do we want Ion and Cranelift to be separate tiers or could we instead:
 - rename Tier::Ion to Tier::Optimized
 - have a bool that indicates whether to use Cranelift for the optimized tier

This would make sense under the assumption that most code that is asking about tier doesn't care about the ion vs. cranelift distinction (since they produce code with the same ABI / metadata).  It seems like mostly a ModuleGenerator-specific concern, but I could be wrong.  What's nice is that this isn't a long-lived distinction; once cranelift "ships", it can go away so the bool that decides which to use could be #ifdef CRANELIFT.

Comment 27

[Luke Wagner [:luke]]

Comment on attachment 9007278 [details] [diff] [review]
4.bindgen-d-types.patch

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

Thanks for breaking out these patches.

::: js/src/wasm/WasmBasicTypes.h
@@ +19,5 @@
> +#ifndef wasm_basic_types_h
> +#define wasm_basic_types_h
> +
> +// This header defines basic enums and structs that don't depend on other header files being
> +// included. It is included by "wasm/WasmTypes.h" AND by bindgen when making bindings for Cranelift.

WasmBinaryConstants.h also doesn't #include anything, could it serve this purpose instead?  It currently contains some structs that maybe trip up bindgen (OpBytes), so perhaps those could be moved to WasmTypes.h?  Also, the "Binary" in "WasmBinaryConstants.h" seems superfluous/confusing, in retrospect, perhaps rename to just WasmConstants.h (which makes it's role here a bit more clear)?

Comment 28

[Lars T Hansen [:lth]]

Comment on attachment 9007289 [details] [diff] [review]
5a-cranelift-integration.patch

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

Seems OK to me, for a first cut.

::: js/src/wasm/cranelift/src/lib.rs
@@ +93,5 @@
> +        error!("Cranelift compilation error: {}\n{}", e, compiler);
> +        return false;
> +    };
> +
> +    // TODO(bbouvier) if destroy is called while one of these objects is alive, you're gonna have a

"going to have"

Comment 29

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #26)
> Comment on attachment 9007275 [details] [diff] [review]
> 3.tier::cranelift.patch
> 
> Review of attachment 9007275 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Question: do we want Ion and Cranelift to be separate tiers or could we
> instead:
>  - rename Tier::Ion to Tier::Optimized
>  - have a bool that indicates whether to use Cranelift for the optimized tier
> 
> This would make sense under the assumption that most code that is asking
> about tier doesn't care about the ion vs. cranelift distinction (since they
> produce code with the same ABI / metadata).  It seems like mostly a
> ModuleGenerator-specific concern, but I could be wrong.  What's nice is that
> this isn't a long-lived distinction; once cranelift "ships", it can go away
> so the bool that decides which to use could be #ifdef CRANELIFT.

That's how it was done in the patch previously, but there was one thing that troubled me: Tier::Serialized == Tier::Ion (and thus would be equal to Tier::Cranelift), so just having Tier::Optimized could mean that Cranelift code gets serialized/deserialized. Since Ion is still generating better code at the moment, I thought not fusing the two would prevent performance issues for people testing cranelift in a browser build.

Also, we could have different heuristics for tiering baseline with cranelift, which is the only other interest.

Reverting to the previous situation would be just a bit bad in my opinion, but I don't care too strongly here.

Comment 30

[Mike Hommey [:glandium]]

Comment on attachment 9007273 [details] [diff] [review]
1.cranelift-nightly.patch

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

::: js/moz.configure
@@ +487,5 @@
>            help='Enable Cranelift code generator for wasm')
>  
> +@depends('--enable-cranelift', milestone)
> +def enable_cranelift(value, milestone):
> +    return value or milestone.is_nightly

This makes --disable-cranelift *not* work on central. We avoid doing things like this. If you want the default to be enabled for central, use default=milestone.is_nightly on the js_option().

Comment 31

[Mike Hommey [:glandium]]

Comment on attachment 9007290 [details] [diff] [review]
5b-build-support.patch

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

::: js/src/wasm/cranelift/moz.build
@@ +3,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +RustLibrary('baldrdash')

Adding if CONFIG['OS_ARCH'] == 'Darwin': OS_LIBS += ['-lresolv'] here should make USE_LIBS += ['baldrdash'] imply it, removing the need to it in all the other files.

Comment 32

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.cranelift-nightly.patch

Thanks! (Also incorporated the change you suggested on the moz.build files in the moz.build patch)

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9347f5a5365d
Add a --wasm-force-cranelift flag to the shell; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7954f956259
Move wasm types shared with Cranelift in a WasmBinaryConstants.h; r=luke

Comment 34

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.cranelift-nightly.patch

Comment 35

[Luke Wagner [:luke]]

Comment on attachment 9007275 [details] [diff] [review]
3.tier::cranelift.patch

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

Ah, Tier::Serialized is a good point.  Moreover, when cranelift ships, we can delete Tier::Ion and so this is only a transitional state.

Comment 36

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/9347f5a5365d
https://hg.mozilla.org/mozilla-central/rev/b7954f956259

Comment 37

[Dan Gohman [:sunfish]]

Comment on attachment 9007289 [details] [diff] [review]
5a-cranelift-integration.patch

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

Looks good to me :-).

Comment 38

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks all for the reviews!

Alright, some status update here: I'm fighting with the build gods at the moment, and have succeeded at solving a few issues. Another bunch of issues needs to be solved before landing.

- build issues: to be linked with libmozjs, baldrdash must have "staticlib" (static library) as its crate-type; to be linked with libxul, it must be a "rlib" (rust object lib) due to the way we link rust code in gecko. These requirements are conflicting: in particular, rust static libs are built with LTO, while rlib can't be. The solution, as suggested by Nathan, was to split baldrdash into two libs:
  - baldrdash, used by Gecko (and baldrdash_lib below), which is an rlib
  - baldrdash_lib, used only by the shell, and defined as a static library, which just references baldrdash.
Thus, baldrdash_lib needs its own Cargo.toml file, and needs to reference the source of baldrdash in its dependencies, so its root directory must be a parent of the baldrdash lib. As a matter of fact, all the code that was in /js/src/wasm/cranelift is now in /js/src/wasm/cranelift/lib; baldrdash_lib's Cargo.toml and dummy lib.rs files live by themselves in /js/src/wasm/cranelift. (Will upload a patch for all the build changes) Happy to change the `lib` name to something else, if people have better suggestions.

- more build issues: the hazard analysis builds don't reference libclang, which is needed by bindgen. Nathan and Steve gave me useful help on IRC to fix this.

- even more build issues: Android builds need more metadata for bindgen. Emilio on irc pointed what needed to be done for the style system, which should be helpful.

- rust-bindgen doesn't support bindings generation for C++ methods attributes on windows: I'll revert to function calls that take what was the value of "this" as the first argument, on all platforms.

- in Firefox, env_logger::init() was called by JS after it was already initialized in Gecko; replaced by a call to try_init() which doesn't fail if a logger has already been set.

Comment 39

[Mike Hommey [:glandium]]

If you create a jsrust staticlib library similar to gkrust, you can then have baldrdash as a rlib linked into either gtkrust or jsrust.

Comment 40

[Mike Hommey [:glandium]]

I should add that linking baldrdash into gkrust is probably not something we can do, not as long as we support building firefox with spidermonkey as a shared library.

Comment 41

[Henri Sivonen (:hsivonen)]

(In reply to Mike Hommey [:glandium] from comment #40)
> I should add that linking baldrdash into gkrust is probably not something we
> can do, not as long as we support building firefox with spidermonkey as a
> shared library.

Why do we need to support that?

That is, is there a reason why we can't aim for a setup where SpiderMonkey-relevant Rust code is built as jsrust in standalone SpiderMonkey builds and as part of gkrust when SpiderMonkey is built as part of Firefox without support for SpiderMonkey as a shared library in Firefox?

Comment 42

[Mike Hommey [:glandium]]

(In reply to Henri Sivonen (:hsivonen) from comment #41)
> (In reply to Mike Hommey [:glandium] from comment #40)
> > I should add that linking baldrdash into gkrust is probably not something we
> > can do, not as long as we support building firefox with spidermonkey as a
> > shared library.
> 
> Why do we need to support that?

People have been using that to make incremental builds faster. Now that there are fast linking options, this might be less necessary, but it's not a given.

> That is, is there a reason why we can't aim for a setup where
> SpiderMonkey-relevant Rust code is built as jsrust in standalone
> SpiderMonkey builds and as part of gkrust when SpiderMonkey is built as part
> of Firefox without support for SpiderMonkey as a shared library in Firefox?

As mentioned in another bug, there's actually another problem, that e.g. the js shell, that is still built for Firefox, is not linking gecko for good reason, and still needs the spidermonkey-specific rust bits.

Comment 43

[Henri Sivonen (:hsivonen)]

(In reply to Mike Hommey [:glandium] from comment #42)
> (In reply to Henri Sivonen (:hsivonen) from comment #41)
> > (In reply to Mike Hommey [:glandium] from comment #40)
> > > I should add that linking baldrdash into gkrust is probably not something we
> > > can do, not as long as we support building firefox with spidermonkey as a
> > > shared library.
> > 
> > Why do we need to support that?
> 
> People have been using that to make incremental builds faster. Now that
> there are fast linking options, this might be less necessary, but it's not a
> given.

That doesn't seems like a great reason to prevent SpiderMonkey from reusing code that Gecko has access to.

> > That is, is there a reason why we can't aim for a setup where
> > SpiderMonkey-relevant Rust code is built as jsrust in standalone
> > SpiderMonkey builds and as part of gkrust when SpiderMonkey is built as part
> > of Firefox without support for SpiderMonkey as a shared library in Firefox?
> 
> As mentioned in another bug, there's actually another problem, that e.g. the
> js shell, that is still built for Firefox, is not linking gecko for good
> reason, and still needs the spidermonkey-specific rust bits.

I looked at a couple of obvious dependencies and didn't find a discussion of js shell issues. Which bug do you mean?

Comment 44

[Mike Hommey [:glandium]]

bug 1490948, I think, but maybe the discussion was only on irc.

Comment 45

[Henri Sivonen (:hsivonen)]

(In reply to Mike Hommey [:glandium] from comment #44)
> bug 1490948, I think, but maybe the discussion was only on irc.

Does anyone remember the details? (Asking in order to understand the issues facing bug 1490601.)

Comment 46

[Benjamin Bouvier [:bbouvier] (inactive)]

Sure, will report there, since I've moved all the build work in this bug.

Comment 47

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3.new-tier.patch

Just wanted a double-check of this patch after the serialization tests landed.

Mostly the same, except we need to explicitly check that the serialized tier isn't the second tier before calling the serialization listener.

Comment 48

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #29)
> > Question: do we want Ion and Cranelift to be separate tiers or could we
> > instead:
> >  - rename Tier::Ion to Tier::Optimized
> >  - have a bool that indicates whether to use Cranelift for the optimized tier

> That's how it was done in the patch previously, but there was one thing that
> troubled me: Tier::Serialized == Tier::Ion (and thus would be equal to
> Tier::Cranelift), so just having Tier::Optimized could mean that Cranelift
> code gets serialized/deserialized. Since Ion is still generating better code
> at the moment, I thought not fusing the two would prevent performance issues
> for people testing cranelift in a browser build.

Actually, thinking about this a bit more, it seems totally fine (and beneficial for testing purposes) to serialize cranelift code.  First of all, it'll only get serialized when the force-cranelift option is set which will only be set for testing purposes.  Second, even if someone sets it and caches some suboptimal cranelift code (before we enable cranelift by default), the code is build-id keyed so as soon as the browser updates we'll compile new code.  So I don't think there is any reason to distinguish Tier::Ion from Tier::Cranelift for serialization purposes.

> Also, we could have different heuristics for tiering baseline with
> cranelift, which is the only other interest.

It's probably fine to just use the Ion constants and then tweak those constants based on re-measurements before cranelift release.

So based on that, how about having just Tier::Baseline and Tier::Optimized?

Comment 49

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 3.new-switch.patch

Here's the patch, which is a conceptual regression I think (more conditions to check to know if we're compiling for Cranelift, meaningless OptimizedBackend enum field in more cases in the CompilerEnv, etc.), but, oh well, I don't want to fight over this.

Comment 50

[Luke Wagner [:luke]]

Comment on attachment 9012112 [details] [diff] [review]
3.new-switch.patch

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

Looks great!

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7df169d72b50
Introduce a new switch to determine if we should compile with Cranelift; r=luke

Comment 52

[Narcis Beleuzu [:NarcisB]]

Backed out for assertion failures on env.optimizedBackend() == OptimizedBackend::Ion. 

Push link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed,busted,exception,runnable
Backout link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed,busted,exception,runnable
Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=201723721&repo=mozilla-inbound&lineNumber=12313

Comment 53

[Narcis Beleuzu [:NarcisB]]

There were also jit failures on WasmCompile.cpp

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=201721372&repo=mozilla-inbound&lineNumber=12285

Comment 54

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b1c85d872cc
Backed out changeset 7df169d72b50 for assertion failures on env.optimizedBackend() == OptimizedBackend::Ion. CLOSED TREE

Comment 55

[Benjamin Bouvier [:bbouvier] (inactive)]

Duh, optimized build + uninitialized memory in !ENABLE_WASM_CRANELIFT build = kaboom. Thanks for the backout.

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2456b585ef87
Introduce a new switch to determine if we should compile with Cranelift; r=luke

Comment 57

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9007290 [details] [diff] [review]
5b-build-support.patch

This work has moved to bug 1490948.

Comment 58

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9007795 [details] [diff] [review]
1.cranelift-nightly.patch

This work has moved to bug 1490948.

Comment 59

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/2456b585ef87

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7864f125bbcd
Integrate Cranelift in Spidermonkey; r=sunfish, r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a644dda1dc3
Vendor in Rust dependencies for Cranelift; r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/66a1434be89e
License changes; r=mhoye

Comment 61

[Benjamin Bouvier [:bbouvier] (inactive)]

Last try build went all green (except for tier2 Btup, to be fixed in bug 1494139): https://treeherder.mozilla.org/#/jobs?repo=try&revision=282c9c7435616db59cd4ad672ea876c883ea3333

Comment 62

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/7864f125bbcd
https://hg.mozilla.org/mozilla-central/rev/8a644dda1dc3
https://hg.mozilla.org/mozilla-central/rev/66a1434be89e

Comment 63

[Blake Winton (:bwinton) (:☕️)]

Hey all,

I'm trying to compile Firefox for the first time on my new MacBook, and am running into the following error:
 9:45.75    Compiling baldrdash v0.1.0 (/Users/bwinton/Programming/mozilla/central/js/src/wasm/cranelift)
 9:46.99 error: failed to run custom build command for `baldrdash v0.1.0 (/Users/bwinton/Programming/mozilla/central/js/src/wasm/cranelift)`
 9:46.99 process didn't exit successfully: `/Users/bwinton/Programming/mozilla/central/obj-x86_64-apple-darwin18.0.0/toolkit/library/release/build/baldrdash-4511ac70b523b040/build-script-build` (exit code: 101)
 9:46.99 --- stdout
 9:46.99 cargo:rerun-if-changed=baldrapi.h
 9:46.99 cargo:rerun-if-changed=/Users/bwinton/Programming/mozilla/central/obj-x86_64-apple-darwin18.0.0/js/src/rust/extra-bindgen-flags
 9:46.99 --- stderr
 9:47.00 /usr/local/Cellar/llvm/7.0.0/lib/clang/7.0.0/include/inttypes.h:30:15: fatal error: 'inttypes.h' file not found
 9:47.00 /usr/local/Cellar/llvm/7.0.0/lib/clang/7.0.0/include/inttypes.h:30:15: fatal error: 'inttypes.h' file not found, err: true
 9:47.00 thread 'main' panicked at 'Unable to generate baldrapi.h bindings: ()', libcore/result.rs:1009:5
 9:47.00 stack backtrace:
 9:47.00    0:        0x105b64e0f - std::sys::unix::backtrace::tracing::imp::unwind_backtrace::heb6afa683932dedf
 9:47.00    1:        0x105b44c7d - std::sys_common::backtrace::print::h766ca70084cc68f0
 9:47.00    2:        0x105b6dae3 - std::panicking::default_hook::{{closure}}::hf3402f90591bf7bb
 9:47.00    3:        0x105b6d86c - std::panicking::default_hook::hb3b7f3a59e2ecd5a
 9:47.00    4:        0x105b6e217 - std::panicking::rust_panic_with_hook::h359374823d95520e
 9:47.00    5:        0x105b6dd7c - std::panicking::continue_panic_fmt::hf199d5eb60536380
 9:47.00    6:        0x105b6dc68 - rust_begin_unwind
 9:47.00    7:        0x105bd20f1 - core::panicking::panic_fmt::he16a30f6c103fffa
 9:47.00    8:        0x104ee97be - core::result::unwrap_failed::h9d03696a608dbad4
 9:47.00    9:        0x104ef0fd5 - build_script_build::main::h0a86b4b58e0c8b60
 9:47.00   10:        0x104ee99f5 - std::rt::lang_start::{{closure}}::he5e0228125c8f22e
 9:47.00   11:        0x105b6dbe7 - std::panicking::try::do_call::h2bd103f24173fc3c
 9:47.00   12:        0x105b7b5fe - __rust_maybe_catch_panic
 9:47.00   13:        0x105b5192c - std::rt::lang_start_internal::h6fb67bdf1de01d4a
 9:47.00   14:        0x104ee99e7 - std::rt::lang_start::h0f8cdf02aa015a6e

It seems like it might be related to this bug, so I wonder if there's something in the bootstrap.py or build setup instructions that needs to be changed to get it compiling?

Thanks!

Comment 64

[Blake Winton (:bwinton) (:☕️)]

(Oh, I should probably also mention that I'm on macOS Mojave, which could very well be the problem.)

Comment 65

[Harry Twyford]

Chiming in that that I'm getting identical output to Blake's after the update to macOS Mojave.

Comment 66

[Nathan Froyd [:froydnj]]

You are both running into bug 1487522.

Comment 67

[Jeff Muizelaar [:jrmuizel]]

Wrong bug number? "Request for Jenkins X VA"

Comment 68

[Nathan Froyd [:froydnj]]

Whoops, bug 1487552.  There we go.