Bug 220365 – [JSC] Allow to build WebAssembly without B3

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Xan Lopez]

Wasm-LLInt can be compiled standalone by disabling at compile time the BBQ/OMG tiers. This is a stepping stone towards a JIT-less Wasm implementation, and useful by itself on platforms without B3/Air support.

WIP patch follows.

[Xan Lopez]

Created attachment 417088 [details]
WIP patch.

WIP patch.

- Adds a WEBASSEMBLY_JIT flag. Technically there's still some JIT used (trampolines), so maybe it could have a different name for now, this just seemed intuitive.

- The patch mostly #ifdefs out the tiering up code. Pretty straightforward save for two bits:

1) code in WasmCallingConvention.h uses B3::ValueRep for all tiers. This seems to be a self-contained class to track the representation of values. It could be moved out of b3/, or we could use something different.

2) the js->wasm entrypoint uses B3::Compilation unconditionally, even for LLInt code. Again, this can be basically compiled by itself, so I'm also doing that. It could be moved elsewhere, or we could use something different.

There's still some JIT usage left, AFAICT used for trampoline code between wasm and js. It's been marked with FIXMEs. If someone manages to also replace that for LLInt-only builds they'd have a JIT-less wasm implementation, I think.

I tried to get the minimal thing that seems to work, but comments are more than welcome about the approach and what to do with the B3 classes that are used unconditionally, I'm far from familiar with the code.

All tests in JSTests/wasm pass.

[Xan Lopez]

(In reply to Xan Lopez from comment #1)
> 
> 1) code in WasmCallingConvention.h uses B3::ValueRep for all tiers. This
> seems to be a self-contained class to track the representation of values. It
> could be moved out of b3/, or we could use something different.
> 
> 2) the js->wasm entrypoint uses B3::Compilation unconditionally, even for
> LLInt code. Again, this can be basically compiled by itself, so I'm also
> doing that. It could be moved elsewhere, or we could use something different.
> 

FWIW, in both cases the code can be compiled by itself and it does not seem to be very B3 specific, at least in how it's used by WebAssembly. ValueRep is used to describe the call information for a wasm function, and Compilation just seems to be a MacroAssemblerCodeRef wrapping a chunk of code. Again, I could be missing something, but this is the reason I think it could make sense to rename and reuse them here.

[Yusuke Suzuki]

Comment on attachment 417088 [details]
WIP patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=417088&action=review

> Source/JavaScriptCore/ChangeLog:11
> +        - Move B3::ValueRep somewhere else?

Let's move it to bytecode/ValueRep.h. bytecode/ is the place to put this kind of data structures which can be used from LLInt etc.
And let's make JSC::B3::ValueRep to JSC::ValueRep if it is used outside of B3.

> Source/JavaScriptCore/ChangeLog:15
> +        - Figure out if we can use something other than B3::Compilation for
> +          the Wasm::Function js->wasm entrypoint (or, maybe, just move
> +          B3::Compilation somewhere else? Seems to be a simple wrapper for a
> +          chunk of code...).

Maybe, let's move B3::Compilation, B3::OpaqueByproduct and B3::OpaqueByproduct to jit/ since they are JIT compilation data.

[Xan Lopez]

Sounds great, thanks for the review! I'll open two bugs to do those changes and then revisit this when that's done.

[Radar WebKit Bug Importer]

<rdar://problem/73152917>

[Xan Lopez]

Created attachment 417833 [details]
Make Wasm/B3 a compile-time option

v1,

once all the refactoring is done the patch is just a matter of adding #ifdefs all over the place.

[Xan Lopez]

Created attachment 417876 [details]
Make Wasm/B3 a compile-time option, v2

v2,
there are failures in the OSX jsc bot, but can't reproduce them locally. Let's try one thing.

[Xan Lopez]

Created attachment 417880 [details]
Make Wasm/B3 a compile-time option, v3

v3,
make WEBASSEMBLY depend on JIT in cmake, and disable explicitly on all 32bit platforms for now.

[Xan Lopez]

Created attachment 417958 [details]
Make Wasm/B3 a compile-time option, v4

v4,
one more try

[Xan Lopez]

Created attachment 418025 [details]
Make Wasm/B3 a compile-time option, v5

v5,
add the missing entry in FeatureList.pm

[Xan Lopez]

Created attachment 418027 [details]
Make Wasm/B3 a compile-time option, v6

v6,
and enable WEBASSEMBLY_B3JIT on PlatformEnable.h for COCOA.

[Mark Lam]

Comment on attachment 418027 [details]
Make Wasm/B3 a compile-time option, v6

View in context: https://bugs.webkit.org/attachment.cgi?id=418027&action=review

r=me with improvements.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:29
> +#if ENABLE(WEBASSEMBLY) && ENABLE(WEBASSEMBLY_B3JIT)

I think you can make this just `ENABLE(WEBASSEMBLY_B3JIT)`.  `ENABLE(WEBASSEMBLY_B3JIT)` should imply `ENABLE(WEBASSEMBLY)`.  Otherwise, it's pointless.

Please also update the comment at the matching `#endif` below with `ENABLE(WEBASSEMBLY_B3JIT)` instead of `ENABLE(WEBASSEMBLY_B3JIT)`.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.h:28
> +#if ENABLE(WEBASSEMBLY) && ENABLE(WEBASSEMBLY_B3JIT)

Ditto here and in other files below.  Also, please update all the corresponding `#endif`s.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:29
> +#if ENABLE(WEBASSEMBLY) && ENABLE(WEBASSEMBLY_B3JIT)

As sanity check, please add the following after the #includes below:

#if !ENABLE(WEBASSEMBLY)
#error ENABLE(WEBASSEMBLY_B3JIT) is enabled, but ENABLE(WEBASSEMBLY) is not.
#endif

This ensures that we'll get a build error if anyone ever misconfigures these flags.  We'll probably get a build error anyway due to use of Wasm data structures, but having this #error will make it more explicit, plus it documents this invariant explicitly.  At first, I thought to put this in PlatformEnable.h, but reconsidered and decided that we don't really need to do this check repeatedly.

> Source/JavaScriptCore/wasm/WasmCallee.h:138
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of text seems long enough that this is warranted.

> Source/JavaScriptCore/wasm/WasmCallee.h:191
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of text seems long enough that this is warranted.

> Source/JavaScriptCore/wasm/WasmCodeBlock.cpp:77
> +#if ENABLE(WEBASSEMBLY_B3JIT)

nit: let's move this `#if` above the `} else {` since the entire else clause if not needed otherwise.

> Source/JavaScriptCore/wasm/WasmCodeBlock.cpp:106
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of text seems long enough that this is warranted.

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.h:32
> +#include "WasmModuleInformation.h"
> +#include "WasmSignature.h"

Let's just forward declare these like we do with the FunctionCodeBlock class below.  It's better to reduce the amount of #include load unless we really need it.

> Source/JavaScriptCore/wasm/WasmOperations.cpp:457
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of text is definitely long enough that this is warranted.

> Source/JavaScriptCore/wasm/WasmPlan.cpp:191
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of text seems long enough that this is warranted.

> Source/JavaScriptCore/wasm/WasmSlowPaths.cpp:258
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of text seems long enough that this is warranted.

> Source/JavaScriptCore/wasm/WasmThunks.cpp:99
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of text seems long enough that this is warranted.

[Xan Lopez]

Created attachment 418062 [details]
Make Wasm/B3 a compile-time option, v7

v7,
also try to enable WebAssembly on FTW/Win, should work up with WasmLLInt.

[Xan Lopez]

Created attachment 418134 [details]
Make Wasm/B3 a compile-time option, v8

v8,
address review comments (including some extra cleanups I saw were possible)
leave the feature disabled for Windows for now, since it's not trivial to enable

[Xan Lopez]

Created attachment 418135 [details]
Make Wasm/B3 a compile-time option, v9

v9,
and one more cleanup

[Xan Lopez]

Created attachment 418141 [details]
Make Wasm/B3 a compile-time option, v10

v10,
of course one of the cleanups broke some builds, try to fix that

[Xan Lopez]

Created attachment 418142 [details]
Make Wasm/B3 a compile-time option, v11

v11,
and another fix for TV/iOS

[Xan Lopez]

Created attachment 418144 [details]
Make Wasm/B3 a compile-time option, v11

v11,
still v11, the fix just did not make it to the patch

[Don Olmstead]

Comment on attachment 418144 [details]
Make Wasm/B3 a compile-time option, v11

View in context: https://bugs.webkit.org/attachment.cgi?id=418144&action=review

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:70
> +#if !ENABLE(WEBASSEMBLY)
> +#error ENABLE(WEBASSEMBLY_B3JIT) is enabled, but ENABLE(WEBASSEMBLY) is not.
> +#endif
> +

You probably want this check in wtf/PlatformEnable.h.

See https://github.com/WebKit/WebKit/blob/main/Source/WTF/wtf/PlatformEnable.h#L861-L881 for examples.

> Source/WTF/wtf/PlatformEnable.h:685
>  #if !defined(ENABLE_WEBASSEMBLY) && (ENABLE(B3_JIT) && PLATFORM(COCOA) && CPU(ADDRESS64))
>  #define ENABLE_WEBASSEMBLY 1
> +#define ENABLE_WEBASSEMBLY_B3JIT 1
>  #endif

Just a thought but would it be better to move this later and do the following?

#if !defined(ENABLE_WEBASSEMBLY_B3JIT) && (ENABLE(WEBASSEMBLY) && ENABLE(B3_JIT))
#define ENABLE_WEBASSEMBLY_B3JIT 1
#endif

[Xan Lopez]

(In reply to Don Olmstead from comment #19)
> Comment on attachment 418144 [details]
> Make Wasm/B3 a compile-time option, v11
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418144&action=review
> 
> > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:70
> > +#if !ENABLE(WEBASSEMBLY)
> > +#error ENABLE(WEBASSEMBLY_B3JIT) is enabled, but ENABLE(WEBASSEMBLY) is not.
> > +#endif
> > +
> 
> You probably want this check in wtf/PlatformEnable.h.

Mark suggested in comment #12 to put it here, so the check does not run multiple times.

> 
> See
> https://github.com/WebKit/WebKit/blob/main/Source/WTF/wtf/PlatformEnable.
> h#L861-L881 for examples.
> 
> > Source/WTF/wtf/PlatformEnable.h:685
> >  #if !defined(ENABLE_WEBASSEMBLY) && (ENABLE(B3_JIT) && PLATFORM(COCOA) && CPU(ADDRESS64))
> >  #define ENABLE_WEBASSEMBLY 1
> > +#define ENABLE_WEBASSEMBLY_B3JIT 1
> >  #endif
> 
> Just a thought but would it be better to move this later and do the
> following?
> 
> #if !defined(ENABLE_WEBASSEMBLY_B3JIT) && (ENABLE(WEBASSEMBLY) &&
> ENABLE(B3_JIT))
> #define ENABLE_WEBASSEMBLY_B3JIT 1
> #endif

This can be a bit confusing at first, but that code only matters for PLATFORM(COCOA) because cmake platforms do this feature dependency tracking/enforcing in the cmake files themselves (see the cmake changes in this patch). And we need to do this in this patch, otherwise WEBASSEMBLY_B3JIT won't be defined and the builds for Mac/iOS/etc will fail.

[EWS]

Committed r271775: <https://trac.webkit.org/changeset/271775>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418144 [details].