Revamp wasm compiler switches [was: Misleading OOM in error message when Cranelift isn't supported]

RESOLVED FIXED in Firefox 66

Status

()

enhancement
RESOLVED FIXED
7 months ago
6 days ago

People

(Reporter: ananuti, Assigned: bbouvier, NeedInfo)

Tracking

(Blocks 1 bug, Regressed 1 bug)

Trunk
mozilla66
x86
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 wontfix, firefox66 fixed)

Details

Attachments

(6 attachments, 9 obsolete attachments)

46.88 KB, patch
lth
: review+
Details | Diff | Splinter Review
8.11 KB, patch
lth
: review+
Details | Diff | Splinter Review
1.34 KB, patch
Details | Diff | Splinter Review
8.99 KB, patch
lth
: review+
Details | Diff | Splinter Review
33.58 KB, patch
lth
: review+
Details | Diff | Splinter Review
19.71 KB, patch
lth
: review+
Details | Diff | Splinter Review
Reporter

Description

7 months ago
I can't squooshing any image with cranelift.

Console:
⚠️ wasm streaming compile failed: out of memory           https://squoosh.app/process-mozjpeg-enc.d4eb5.worker.js:1:9135
⚠️ falling back to ArrayBuffer instantiation              process-mozjpeg-enc.d4eb5.worker.js:1:9174
⚠️ failed to asynchronously prepare wasm: out of memory   process-mozjpeg-enc.d4eb5.worker.js:1:8877
⚠️ out of memory                                          process-mozjpeg-enc.d4eb5.worker.js:1:29451
⚠️ out of memory                                          process-mozjpeg-enc.d4eb5.worker.js:1:29456
Assignee

Comment 1

7 months ago
Thanks for the report! Is there any chance you're using an x86 32 bits build of Firefox? Cranelift is officially only support on x86 64 bits so far, and we will report an out-of-memory error in other cases. If that's so, we can probably report a better error message for this.
Flags: needinfo?(ananuti)
Reporter

Comment 2

7 months ago
(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> Thanks for the report! Is there any chance you're using an x86 32 bits build
> of Firefox? 

I'm on x86 32 bits build.

> Cranelift is officially only support on x86 64 bits so far, 

How sad!! o(╥﹏╥)o

> and we will report an out-of-memory error in other cases. If that's so, we can
> probably report a better error message for this.

That'd be nice. (◕‿◕✿)
Flags: needinfo?(ananuti)
Assignee

Comment 3

7 months ago
Thanks for the answer! I'll see what I can do.
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Assignee

Updated

7 months ago
Summary: Cranelift: OOM on https://squoosh.app → Misleading OOM in error message when Cranelift isn't supported
Assignee

Comment 4

7 months ago
Posted patch 1.rename-compileargs.patch (obsolete) — Splinter Review
This does a few renamings which made sense to me (enabled felt a bit vague to me in general). I'm open to discussions here.
Attachment #9028935 - Flags: review?(lhansen)
Assignee

Comment 5

7 months ago
This updates CompilerEnv::computeParameters in subtle ways to make it easier to understand; in particular the impact of debugEnabled || gcEnabled is made very clear.
Attachment #9028936 - Flags: review?(lhansen)
Assignee

Comment 6

7 months ago
Posted patch 3.craneliftcancompile.patch (obsolete) — Splinter Review
Adds a CraneliftCanCompile() function, which is used to know if a specific platform is supported. Also disentangles the wasm-ion pref and the wasm-cranelift pref in the following manner:

- if wasm-ion is false, wasm-cranelift is true, then Cranelift is to be used (as the main backend or second tier). That's the new thing. People have been confused in the past that wasm-ion should be enabled so as to use Cranelift.
- if wasm-ion is true, wasm-cranelift is true, then Cranelift is to be used (as was done before this patch).
Attachment #9028937 - Flags: review?(lhansen)
Assignee

Comment 7

7 months ago
Posted patch 4.warning.patch (obsolete) — Splinter Review
Adds a warning to tell when compiler options are overridden. This could be merged with and included in the verbose mode message when we get to implement it.

/tmp/a.js:1:1 warning: WebAssembly module validated with warning: Note: Cranelift isn't supported on this platform, using another backend.

It's unfortunate because this will only emit a warning when Cranelift isn't supported, instead of failing to compile, but this is more in line with what we do with overrides, and it might be confusing that only the second tier fails to compile with an error in a two-tier compilation setup.
Attachment #9028939 - Flags: review?(lhansen)
Assignee

Comment 8

7 months ago
An exploration I made to try to construct CompileArgs in an immutable way, well almost immutable since these compiler options are overridden if there's a wasm gc section, but that's not looking much better in my opinion, so I gave it up.
Comment on attachment 9028937 [details] [diff] [review]
3.craneliftcancompile.patch

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

::: js/src/wasm/WasmCompile.cpp
@@ +97,5 @@
> +        // assume false for the other options.
> +        sharedMemoryEnabled = false;
> +        debugEnabled = false;
> +        return;
> +    }

As a style matter, I would probably prefer for you to remove the "return" above and put the following two statements into an "else", to make sure that future changes is not confused about what looks like an early exit here.
Attachment #9028937 - Flags: review?(lhansen) → review+
Comment on attachment 9028935 [details] [diff] [review]
1.rename-compileargs.patch

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

I'm not convinced the naming change makes much of a difference in clarity, but if you're happier with it then I'm fine with it.

You should move the prose that is in the commit message now into a comment in the code, so that it's easier for future users to understand what the naming convention is.
Attachment #9028935 - Flags: review?(lhansen) → review+
Comment on attachment 9028936 [details] [diff] [review]
2.simplify-computeparameters.patch

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

::: js/src/wasm/WasmCompile.cpp
@@ +434,5 @@
> +
> +    if (debugEnabled || gcEnabled) {
> +        forceTiering = false;
> +        shouldUseIon = false;
> +        shouldUseBaseline = true;

Really should set shouldUseCranelift = false since cranelift won't work with debugging or gc.

Also it makes me very nervous that we're using forceTiering above and then setting it to a different value here.  Control flow allows that to be correct but it would be better to declare the four variables (uninitialized), then have an if/else nest here that sets all four.
Attachment #9028936 - Flags: review?(lhansen) → review+
Comment on attachment 9028939 [details] [diff] [review]
4.warning.patch

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

Clearing review on this until the others have landed and we can see the cumulative effects of those changes, I'm not convinced yet that these messages will be helpful or sufficiently rare to be treated as anything except noise.

::: js/src/wasm/WasmCompile.cpp
@@ +443,5 @@
>      bool shouldUseCranelift = args_->shouldUseCranelift;
>  
>      if (debugEnabled || gcEnabled) {
> +        d.warnf("Note: GC support and debugging support disable compilation by Ion/Cranelift, "
> +                "baseline will be used instead.");

IMO this is going to add a lot of noise that will be incomprehensible to most users, even once we don't need to worry about gcEnabled.
Attachment #9028939 - Flags: review?(lhansen)

Updated

6 months ago
See Also: → 1515623

Updated

6 months ago
Summary: Misleading OOM in error message when Cranelift isn't supported → Revamp wasm compiler switches [was: Misleading OOM in error message when Cranelift isn't supported]
Summarizing recent developments:

In general we are in a fairly messy situation because we have three compilers:

- baseline supports wasm reftypes and wasm gc types but not caching(*) or asm.js
- ion supports wasm reftypes (soon) and asm.js and caching, but not wasm gc types
- cranelift supports neither reftypes, gc, asm.js, or caching(*)

(*) as in, we should not cache this code.

In addition, --wasm-gc is about to get a new meaning (bug 1488205), (gc_feature_opt_in n) complicates it further (ditto), and --test-wasm-await-tier2 further complicates it because it seems to force-reenable ion (see bug 1515623).

We have command line switches that control availability and various TestingFunctions that test availability, but these test static availability, yet for a number of test cases we care about what will be used, dynamically, to compile the code.  For example, reftypes tests can use baseline or ion but not cranelift; "gc type" tests can use only baseline; etc.

An argument can be made that this is a temporary situation but it would be nice for things to be sane and well-tested even in the short term.

As a stopgap I can use wasmCompileMode dynamically to guard test cases, eg, if the mode is "baseline" (only baseline is available) then I run struct tests.  (See bug 1508561.)  However this guard only works if we run all tests with --wasm-no-ion, which we don't, because bug 1515085 prevents it.

Updated

6 months ago
See Also: → 1515917

Updated

6 months ago
Blocks: 1515623
Assignee

Comment 14

6 months ago
(In reply to Lars T Hansen [:lth] from comment #10)
> Comment on attachment 9028935 [details] [diff] [review]
> 1.rename-compileargs.patch
> 
> Review of attachment 9028935 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not convinced the naming change makes much of a difference in clarity,
> but if you're happier with it then I'm fine with it.

I'd like the naming to be clearer for everyone, including you. Linking with the wording used in your latest comment:
1. what's dynamically allowed (mayUseX). For instance, you may use baseline if we haven't disabled it via --wasm-force-ion.
2. what should be used to satisfy extra constraints, independently of static availability (shouldUseX). That's the conjunction of mayUseX and other flags that force things (for instance, wasm force tiering implies shouldUseBaseline and shouldUseIon, even though baseline was not allowed; debug/GC implies baseline, etc.).
3. what's eventually used when you blend in static availability (useX). That's the conjunction of shouldUseX with the result of the {Baseline,Ion,Cranelift}CanCompile() functions.

I'm happy to rename all these things, if people have better ideas. Or I could put this explanation in the commit message and/or code, since it's even more comprehensive and contains examples.

> An argument can be made that this is a temporary situation but it would be nice for things to be sane and well-tested even in the short term.

Yes and no. In the limit, 3) should just be the same as shouldUseX (except for exotic tier-3 platforms). But 1) might remain because of independent unit testing; 2) might remain because of debugging and testing.

I really think the source of confusion is 2), because it does silently overrides what we chose to use. A suggested alternative would be a combination of explicitly throwing if there's a contradiction, and using more testing functions to know if we can do things, which would be more explicit and avoid bugs in general. (For instance: if a test declares --force-wasm-tiering and --no-wasm-baseline, then it would throw because the latter flag contradicts the former; but a canWasmTier() function is introduced and called early, exiting the test since there's a contradiction in the testing flags.) This solution would be better for the long time, since it doesn't introduce silent overrides. In the initial context of this bug, it means using Cranelift on x86 32 bits would throw, which is explicitly nice, as opposed to the final state after my patches (Cranelift is silently disabled, or disabled with a console message if we finally decide to take the fourth path).

Luke, Lars, what do you think? Should we go on with the current patches now, and move on to explicit throwing later? Or do explicit throwing now?
Flags: needinfo?(luke)
Flags: needinfo?(lhansen)

I like the principles of never enabling anything that was explicitly disabled by a flag and having sufficient wasmXEnabled() for all interesting X to use in "|jit-test| skip-if:" and inside tests to deal with the consequences.

Also, building on what we already have and trying to remove the "should"/"may"/"use" distinction, how about:

  • "XSupported" means "supported by the build/hardware; impossible to use if not supported" (iiuc, the current meaning)
  • "XFlag" reflects the (default or explicit) bool flag value of an about:config pref or shell flag.
  • "XEnabled" === "XSupported && XFlag" (is this the current meaning?)

Considering the outliers:

  • Maybe we can simply kill the GC opt-in header, even before we have Ion+GC support by saying that, until GC works in Ion, GC is only enabled when WasmBaselineEnabled && !WasmIonEnabled && !WasmCraneliftEnabled. Then all the GC tests have skip-if: !wasmGCEnabled().
  • --test-wasm-await-tier2 and wasm.delay-tier2 have no effect if tiering is not enabled due to other flags. Tests that want to assert something about tiers should skip-if tiering is not enabled (tested by some new wasmTieringIsEnabled()).

Would that work?

Flags: needinfo?(luke)

(In reply to Luke Wagner [:luke] from comment #15)

I like the principles of never enabling anything that was explicitly disabled by a flag and having sufficient wasmXEnabled() for all interesting X to use in "|jit-test| skip-if:" and inside tests to deal with the consequences.

Also, building on what we already have and trying to remove the "should"/"may"/"use" distinction, how about:

  • "XSupported" means "supported by the build/hardware; impossible to use if not supported" (iiuc, the current meaning)
  • "XFlag" reflects the (default or explicit) bool flag value of an about:config pref or shell flag.
  • "XEnabled" === "XSupported && XFlag" (is this the current meaning?)

This seems reasonable, and I'm also generally in favor of fail-fast.

Considering the outliers:

  • Maybe we can simply kill the GC opt-in header, even before we have Ion+GC support by saying that, until GC works in Ion, GC is only enabled when WasmBaselineEnabled && !WasmIonEnabled && !WasmCraneliftEnabled. Then all the GC tests have skip-if: !wasmGCEnabled().

The opt-in header serves another main purpose, namely, to make sure compiled wasm code is compatible with the prototype encodings used by the engine running that code. Without this you'd get all sorts of mysterious breakage. I want to not require the header for reftypes asap, but it will IMO need to be required for other gc feature things that are not stable.

It is possible that --wasm-gc could just (explicitly) be an alias for --no-wasm-ion --no-wasm-cranelift, now that the suppressGC hacks have been removed, although I don't think I'm in favor of it. At the moment, we get to test that the gc bits that are currently only supported by the baseline compiler interact properly with ion-compiled code, in multi-module situations. That seems useful.

  • --test-wasm-await-tier2 and wasm.delay-tier2 have no effect if tiering is not enabled due to other flags. Tests that want to assert something about tiers should skip-if tiering is not enabled (tested by some new wasmTieringIsEnabled()).

My preference would be for an early exit if a flag is set that can't be honored, ie, not "no effect" but "abort". I guess this is harder in the browser though, so "no effect" is a workable compromise.

(In reply to Benjamin Bouvier [:bbouvier] from comment #14)

(In reply to Lars T Hansen [:lth] from comment #10)

Comment on attachment 9028935 [details] [diff] [review]
1.rename-compileargs.patch

I really think the source of confusion is 2), because it does silently
overrides what we chose to use.

I agree.

A suggested alternative would be a
combination of explicitly throwing if there's a contradiction, and using
more testing functions to know if we can do things, which would be more
explicit and avoid bugs in general.

I agree with that too, although "throwing" is hard sometimes in the context of the browser.

(For instance: if a test declares
--force-wasm-tiering and --no-wasm-baseline, then it would throw because the
latter flag contradicts the former; but a canWasmTier() function is
introduced and called early, exiting the test since there's a contradiction
in the testing flags.)

So I guess the "throwing" would be happen on the first-tier compilation, "new WebAssembly.Module" et al. This seems reasonable to me.

This solution would be better for the long time,
since it doesn't introduce silent overrides. In the initial context of this
bug, it means using Cranelift on x86 32 bits would throw, which is
explicitly nice, as opposed to the final state after my patches (Cranelift
is silently disabled, or disabled with a console message if we finally
decide to take the fourth path).

Right.

Luke, Lars, what do you think? Should we go on with the current patches now,
and move on to explicit throwing later? Or do explicit throwing now?

IMO let's leap into the future and implement sanity checks that throw, when we can. (Also we need to do better when we test, see bug 1515917 for ugly details.)

Flags: needinfo?(lhansen)
Assignee

Comment 18

5 months ago

Thanks for your input! I like the flag/supported/enabled naming (it's clearer than may, etc).

That being said (and sorry in advance for the long post), going back to this, I have a few questions.

  1. It seems there's a confusion related to what should --wasm-force-cranelift do, since it's used in two contexts.

A. Does it mean "use Cranelift when Ion would be used"? Which is the current behavior, and so we do not really "force" usage of Cranelift, since the baseline can be used in practice. This has led me to confusion in the past, where I thought I was testing Cranelift, although Spidermonkey would sometimes run baseline.
B. Does it mean "we want to use Cranelift unconditionally, and not tier"?

It's actually even a bit deeper than this: --no-wasm-ion and --no-wasm-baseline worked fine as long as there were only 2 compilers. In the future if/when cranelift is enabled by default, --no-wasm-ion can mean "force baseline all the time" or "baseline as a first tier, cranelift as a second one if it's available" (depending on whether we enable the Cranelift flag).

It sounds tempting to replace the --no-wasm-* switches too, so that we have:

  • --wasm-unique-backend={ion,cranelift,baseline} (or --wasm-always-backend) to mean we want to test only one backend. Then --no-wasm-ion (resp baseline) is equivalent to --wasm-unique-backend=baseline (resp ion) in current terms, and we can also request testing only Cranelift.
  • --wasm-tier-backend={ion,cranelift} to tell which backend can be used as the second tier backend.

It would be an error to use both flags at the same time.

Using GC or debug would require --wasm-unique-backend and throw otherwise.

Forcing tiering would require --wasm-tier-backend (and per the first rule, would be an error to use with --wasm-unique-backend).

  1. In addition to this --wasm-force-cranelift issue, I think the proper way to deal with this is to construct CompileArgs immutably, in a factory style (CompileArgs::make(JSContext*, ScriptedCaller&&)). Then, one can't reset the fields arbitrarily, and the structure is immutably built (with factory variants for other threads where we don't have a JSContext and guess what's actually possible, and then check at instantiation). I tried this in the past, but this couldn't work without explicit throwing if two flags were contradicting each other. Now that we've agreed upon explicitly throwing, this should be possible to do.

  2. Also, asked Lars on IRC if we had some benefits from using HasGcTypes::{True/False} and not just a plain bool, and we both agreed it could just be a boolean.

How do these proposals sound to you?

I agree the time has come for the --no-wasm-whatever switches to go.

Could we roll the compiler selection switches into one?

--wasm-compiler=baseline+ion
--wasm-compiler=baseline
--wasm-compiler=baseline+cranelift
--wasm-compiler=none

I ask because this might simplify testing somewhat and it would make it clear what is being asked, and it matches (or could match) what wasmCompilationPolicy() is returning. The keyword can be none, baseline, ion, cranelift, baseline+ion or baseline+cranelift. It can't be nothing. The default is baseline+ion except on ARM64, where it is baseline.

Now as you say, --wasm-gc would (temporarily) require also --wasm-compiler=baseline, or it could imply it and it could error out if another value was provided. And forcing tiering would require a baseline+something pair.

Assignee

Comment 20

5 months ago

See commit message. The code is much easier to read this way in my opinion.

Attachment #9028935 - Attachment is obsolete: true
Attachment #9028936 - Attachment is obsolete: true
Attachment #9028937 - Attachment is obsolete: true
Attachment #9028939 - Attachment is obsolete: true
Attachment #9028940 - Attachment is obsolete: true
Attachment #9038282 - Flags: review?(lhansen)
Assignee

Comment 21

5 months ago
Posted patch 2.renaming.patch (obsolete) — Splinter Review

Using the flag/enabled terms; it feels clearer. There's a need for another patch for the peculiar case of gcTypesEnabled, which is a mix of checking the flag and support.

Attachment #9038294 - Flags: review?(lhansen)
Assignee

Comment 22

5 months ago
Comment on attachment 9038294 [details] [diff] [review]
2.renaming.patch

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

With subsequent patches, there's no need for this one anymore.
Attachment #9038294 - Flags: review?(lhansen)
Assignee

Comment 24

5 months ago
Posted patch 3.wasm-compiler-switch.patch (obsolete) — Splinter Review
Assignee

Comment 25

5 months ago

(not to be checked in, but helped to find things i broke)

Assignee

Comment 28

5 months ago
Attachment #9038540 - Attachment is obsolete: true
Assignee

Comment 29

5 months ago
Attachment #9038543 - Attachment is obsolete: true
Attachment #9038556 - Flags: review?(lhansen)
Assignee

Updated

5 months ago
Attachment #9038542 - Flags: review?(lhansen)
Assignee

Updated

5 months ago
Attachment #9038555 - Flags: review?(lhansen)
Assignee

Updated

5 months ago
Attachment #9038539 - Flags: review?(lhansen)
Assignee

Comment 30

5 months ago

Sorry for the patch ordering, spotted nits in patch 3.

So after this series of patches, the --no-wasm, --no-wasm-baseline, --no-wasm-ion, --wasm-force-cranelift switches have been updated to a single one, as suggested by Lars: --wasm-compiler={none,baseline,baseline+ion,ion,cranelift,baseline+cranelift}.

The last patch implements consistency checking between the --wasm-compiler switch and other flags. The default value hasn't changed: wasm is enabled, as well as baseline and ion. See examples below, where a.js is a script compiling a single wasm module (no GC opt-int) and gc.js is the same with a GC feature opt-in:

➜ d64 ./dist/bin/js /tmp/a.js
one shot compilation with ion
➜ d64 ./dist/bin/js --wasm-compiler=baseline /tmp/a.js
one shot compilation with baseline
➜ d64 ./dist/bin/js --wasm-compiler=ion /tmp/a.js
one shot compilation with ion
➜ d64 ./dist/bin/js --wasm-compiler=cranelift /tmp/a.js
one shot compilation with cranelift
➜ d64 ./dist/bin/js --wasm-compiler=baseline+cranelift /tmp/a.js
one shot compilation with cranelift
➜ d64 ./dist/bin/js --wasm-compiler=baseline+ion /tmp/a.js
one shot compilation with ion
➜ d64 ./dist/bin/js --wasm-compiler=baseline+ion --test-wasm-await-tier2 /tmp/a.js
two-tiered compilation starting with baseline
➜ d64 ./dist/bin/js --wasm-compiler=baseline --test-wasm-await-tier2 /tmp/a.js
/tmp/a.js:1:1 Error: can't force tiering without both baseline and a second tier
Stack:
@/tmp/a.js:1:1
➜ d64 ./dist/bin/js --wasm-compiler=baseline+cranelift --test-wasm-await-tier2 /tmp/a.js
two-tiered compilation starting with baseline

➜ d64 ./dist/bin/js --wasm-compiler=baseline --wasm-gc /tmp/gc.js
one shot compilation with baseline
➜ d64 ./dist/bin/js --wasm-compiler=ion --wasm-gc /tmp/gc.js
/tmp/gc.js:1:1 Error: can't use wasm debug/gc without baseline
Stack:
@/tmp/gc.js:1:1
➜ d64 ./dist/bin/js --wasm-compiler=baseline+ion --wasm-gc /tmp/gc.js
one shot compilation with baseline
➜ d64 ./dist/bin/js --wasm-compiler=baseline+ion --wasm-gc --test-wasm-await-tier2 /tmp/gc.js
one shot compilation with baseline
// In this case, test wasm await tier 2 is silently overriden, because wasm-gc already forces the baseline compiler.
➜ d64 ./dist/bin/js --wasm-compiler=cranelift --wasm-gc /tmp/gc.js
/tmp/gc.js:1:1 Error: can't use wasm debug/gc without baseline
Stack:
@/tmp/gc.js:1:1

I am building the whole browser now to make sure this will (actually!) solve the initial issue, that is, that we raise an error when Cranelift isn't natively supported.

Assignee

Comment 31

5 months ago

I could confirm that disabling baseline, enabling wasm-gc and cranelift leads to an alert() with the Spidermonkey error on the WebAssembly.org demo.

Assignee

Comment 32

5 months ago

A small change in functionality, because there was one more workflow I didn't test and wanted to support: jit_tests.py --args=--wasm-compiler=ion wasm, for instance.

Some tests hard fail under debug in this case, others because of --test-wasm-await-tier2. For the debug tests, we can just add a skip-if directive in the test; for the tier2 testing, unfortunately there's not much better than silently ignoring the switch, if it's given (we could alternatively have it be the equivalent of --wasm-compiler=baseline+ion, but it would still interfere badly with --wasm-gc). Updated the patch to reflect this.

Attachment #9038556 - Attachment is obsolete: true
Attachment #9038556 - Flags: review?(lhansen)
Attachment #9038572 - Flags: review?(lhansen)
Comment on attachment 9038282 [details] [diff] [review]
1.bool-hasgctypes.patch

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

A couple of English nits & grammar in the commit comment, I suggest this for the first paragraph:

```
The custom bool enum types are usually created to explicate the role of
arguments at call sites. That being said, they make the code less readable if
they're used a lot, which is the case for HasGcTypes. Moreover, since
literal bool values are only passed in a few places, we are better off using
an inline comment than having a type just for this purpose.
```

::: js/src/wasm/WasmOpIter.h
@@ +591,1 @@
>        two.isReference()) {

Merge lines?

@@ +767,5 @@
>    StackType expected = StackType(expectedType);
>  
>    if (!MOZ_UNLIKELY(observed == expected)) {
>      if (observed == StackType::TVar ||
> +        (env_.gcTypesEnabled() && observed.isReference() &&

Split line?
Attachment #9038282 - Flags: review?(lhansen) → review+
Comment on attachment 9038539 [details] [diff] [review]
2.simplify-compileargs-fields-computeparameters.patch

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

Seems reasonable.

::: js/src/wasm/WasmCompile.cpp
@@ +420,5 @@
> +  bool baselineEnabled = BaselineCanCompile() && baselineFlag;
> +  bool debugEnabled = BaselineCanCompile() && debugFlag;
> +  bool ionEnabled = IonCanCompile() && (ionFlag || !baselineEnabled);
> +#ifdef ENABLE_WASM_CRANELIFT
> +  bool craneliftFlag = args_->craneliftEnabled;

Move craneliftFlag up with the other flags?
Attachment #9038539 - Flags: review?(lhansen) → review+
Comment on attachment 9038555 [details] [diff] [review]
3.wasm-compiler-switch.patch

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

Sweet.
Attachment #9038555 - Flags: review?(lhansen) → review+

Updated

5 months ago
Attachment #9038542 - Flags: review?(lhansen) → review+
Comment on attachment 9038572 [details] [diff] [review]
5.return-errors-and-allow-cranelift.patch

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

Nice work.

::: js/src/builtin/TestingFunctions.cpp
@@ +687,5 @@
>  }
>  
> +static bool WasmDebugSupport(JSContext* cx, unsigned argc, Value* vp) {
> +  CallArgs args = CallArgsFromVp(argc, vp);
> +  args.rval().setBoolean(cx->options().wasmBaseline());

For this predicate, I *think* we want `&& BaselineCanCompile()` since the baseline compiler sometimes requires hardware features that are not on every platform.  But I'm not 100% sure what will happen then.  Mostly this change would be a no-op but ARMv7 systems without IDIV would return false for wasmDebugSupport, for example.  I think that would just cause some debug tests not to run, since wasmDebugSupport is not used for anything else.

::: js/src/wasm/WasmCompile.cpp
@@ +76,5 @@
>  SharedCompileArgs
>  CompileArgs::build(JSContext* cx, ScriptedCaller&& scriptedCaller)
>  {
> +  bool baseline = BaselineCanCompile() && cx->options().wasmBaseline();
> +  bool ion = IonCanCompile() && cx->options().wasmIon();

Yes indeed.  I was initially thinking that these guards should have been in the options processing in js.cpp but this is the better place (and js.cpp is not used in the browser obviously).

@@ +105,5 @@
>  
> +  if (forceTiering && (!baseline || (!cranelift && !ion))) {
> +    // This can happen only in testing, and in this case we don't have a
> +    // proper way to signal the error, so just silently override the default,
> +    // instead of adding a skip-if directive to every test using debug/gc.

Printing a warning would just be annoying, I guess?
Attachment #9038572 - Flags: review?(lhansen) → review+
Assignee

Comment 37

5 months ago

(In reply to Lars T Hansen [:lth] from comment #36)

Comment on attachment 9038572 [details] [diff] [review]
5.return-errors-and-allow-cranelift.patch

Review of attachment 9038572 [details] [diff] [review]:

Nice work.

Thanks for the quick reviews!

(style nits in the first review not addressed since mach clang format will take care of this; I learnt yesterday it's run once a week by release managers)

::: js/src/builtin/TestingFunctions.cpp
@@ +687,5 @@

}

+static bool WasmDebugSupport(JSContext* cx, unsigned argc, Value* vp) {

  • CallArgs args = CallArgsFromVp(argc, vp);
  • args.rval().setBoolean(cx->options().wasmBaseline());

For this predicate, I think we want && BaselineCanCompile() since the
baseline compiler sometimes requires hardware features that are not on every
platform. But I'm not 100% sure what will happen then. Mostly this change
would be a no-op but ARMv7 systems without IDIV would return false for
wasmDebugSupport, for example. I think that would just cause some debug
tests not to run, since wasmDebugSupport is not used for anything else.

I've added this. We might need to review all the places where we use the options() flags and consider this to be the ultimate truth of whether the actual backend is enabled or not, because we've had a few issues with this in the past.

@@ +105,5 @@

  • if (forceTiering && (!baseline || (!cranelift && !ion))) {
  • // This can happen only in testing, and in this case we don't have a
  • // proper way to signal the error, so just silently override the default,
  • // instead of adding a skip-if directive to every test using debug/gc.

Printing a warning would just be annoying, I guess?

We could do it, but warnings are disabled by default, and they would not show up locally or in automation, which is why I decided to just silently ignore the option in this case. So keeping no warning, as you agreed with this on IRC.

(In reply to Benjamin Bouvier [:bbouvier] from comment #37)

(In reply to Lars T Hansen [:lth] from comment #36)

+static bool WasmDebugSupport(JSContext* cx, unsigned argc, Value* vp) {

  • CallArgs args = CallArgsFromVp(argc, vp);
  • args.rval().setBoolean(cx->options().wasmBaseline());

For this predicate, I think we want && BaselineCanCompile() since the
baseline compiler sometimes requires hardware features that are not on every
platform. But I'm not 100% sure what will happen then. Mostly this change
would be a no-op but ARMv7 systems without IDIV would return false for
wasmDebugSupport, for example. I think that would just cause some debug
tests not to run, since wasmDebugSupport is not used for anything else.

I've added this. We might need to review all the places where we use the options() flags and consider this to be the ultimate truth of whether the actual backend is enabled or not, because we've had a few issues with this in the past.

This is how HasReftypesSupport got introduced: so that we could always just use that instead of checking the options(). There could be HasWasmBaselineCompiler() similarly.

Comment 39

5 months ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9ffcdc09fdd
Remove HasGcTypes boolean type and just use bool instead; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/85c33286abbf
Simplify CompileArgs field names and CompilerEnv::computeParameters; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/48dc14f79fb0
Unify wasm compilation switches under the --wasm-compiler umbrella; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/677eda6bdaf9
Introduce a factory function to build immutable CompileArgs; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb20dcd8ea7e
Check coherency of compiler switches when building a CompilerArgs; r=lth

Comment 40

5 months ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7df604faea71
Make sure non-Cranelift builds still work; rs=lth
Assignee

Comment 41

5 months ago

Decoder, Gary: note this updates shell flags (as reflected in shell/fuzz-flags.txt):

  • --no-wasm, --no-wasm-baseline and --no-wasm-ion have been removed
  • everything has been replaced with --wasm-compiler, which can take any of the following values: none, baseline, ion+baseline, ion, cranelift, ion+cranelift. All of them but the last two would be nice to test.

Thanks!

Flags: needinfo?(nth10sd)
Flags: needinfo?(choller)

Drive-by pedantry:

Actually, "baseline+ion" and "baseline+cranelift", not "ion+baseline" and "ion+cranelift".

(In reply to Benjamin Bouvier [:bbouvier] from comment #41)

  • --no-wasm, --no-wasm-baseline and --no-wasm-ion have been removed
  • everything has been replaced with --wasm-compiler, which can take any of the following values: none, baseline, ion+baseline, ion, cranelift, ion+cranelift. All of them but the last two would be nice to test.

Thanks for the needinfo! I quickly added this to funfuzz as things were breaking all round...

https://github.com/MozillaSecurity/funfuzz/commit/e0afd61b4b6201558de5578e5c5cb07e6dce8c30

Flags: needinfo?(nth10sd)
Depends on: 1522824
Regressions: 1558165
You need to log in before you can comment on or make changes to this bug.