Bug 220365 - [JSC] Allow to build WebAssembly without B3
Summary: [JSC] Allow to build WebAssembly without B3
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 220412 220585
Blocks:
  Show dependency treegraph
 
Reported: 2021-01-06 08:48 PST by Xan Lopez
Modified: 2021-01-23 04:32 PST (History)
19 users (show)

See Also:


Attachments
WIP patch. (41.16 KB, patch)
2021-01-06 08:58 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
Make Wasm/B3 a compile-time option (30.65 KB, patch)
2021-01-18 08:24 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
Make Wasm/B3 a compile-time option, v2 (30.73 KB, patch)
2021-01-19 08:02 PST, Xan Lopez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Make Wasm/B3 a compile-time option, v3 (32.11 KB, patch)
2021-01-19 09:23 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
Make Wasm/B3 a compile-time option, v4 (32.05 KB, patch)
2021-01-20 04:59 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
Make Wasm/B3 a compile-time option, v5 (32.24 KB, patch)
2021-01-21 02:53 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
Make Wasm/B3 a compile-time option, v6 (32.58 KB, patch)
2021-01-21 03:05 PST, Xan Lopez
mark.lam: review+
mark.lam: commit-queue-
Details | Formatted Diff | Diff
Make Wasm/B3 a compile-time option, v7 (34.33 KB, patch)
2021-01-21 10:33 PST, Xan Lopez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Make Wasm/B3 a compile-time option, v8 (34.69 KB, patch)
2021-01-22 04:21 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
Make Wasm/B3 a compile-time option, v9 (35.13 KB, patch)
2021-01-22 04:25 PST, Xan Lopez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Make Wasm/B3 a compile-time option, v10 (35.39 KB, patch)
2021-01-22 06:03 PST, Xan Lopez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Make Wasm/B3 a compile-time option, v11 (35.39 KB, patch)
2021-01-22 06:10 PST, Xan Lopez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Make Wasm/B3 a compile-time option, v11 (35.39 KB, patch)
2021-01-22 06:17 PST, Xan Lopez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xan Lopez 2021-01-06 08:48:29 PST
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.
Comment 1 Xan Lopez 2021-01-06 08:58:15 PST
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.
Comment 2 Xan Lopez 2021-01-06 09:14:33 PST
(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.
Comment 3 Yusuke Suzuki 2021-01-06 19:07:38 PST
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.
Comment 4 Xan Lopez 2021-01-07 01:07:01 PST
Sounds great, thanks for the review! I'll open two bugs to do those changes and then revisit this when that's done.
Comment 5 Radar WebKit Bug Importer 2021-01-13 08:49:21 PST
<rdar://problem/73152917>
Comment 6 Xan Lopez 2021-01-18 08:24:03 PST
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.
Comment 7 Xan Lopez 2021-01-19 08:02:53 PST
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.
Comment 8 Xan Lopez 2021-01-19 09:23:47 PST
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.
Comment 9 Xan Lopez 2021-01-20 04:59:14 PST
Created attachment 417958 [details]
Make Wasm/B3 a compile-time option, v4

v4,
one more try
Comment 10 Xan Lopez 2021-01-21 02:53:46 PST
Created attachment 418025 [details]
Make Wasm/B3 a compile-time option, v5

v5,
add the missing entry in FeatureList.pm
Comment 11 Xan Lopez 2021-01-21 03:05:55 PST
Created attachment 418027 [details]
Make Wasm/B3 a compile-time option, v6

v6,
and enable WEBASSEMBLY_B3JIT on PlatformEnable.h for COCOA.
Comment 12 Mark Lam 2021-01-21 10:17:45 PST
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.
Comment 13 Xan Lopez 2021-01-21 10:33:32 PST
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.
Comment 14 Xan Lopez 2021-01-22 04:21:10 PST
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
Comment 15 Xan Lopez 2021-01-22 04:25:49 PST
Created attachment 418135 [details]
Make Wasm/B3 a compile-time option, v9

v9,
and one more cleanup
Comment 16 Xan Lopez 2021-01-22 06:03:05 PST
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
Comment 17 Xan Lopez 2021-01-22 06:10:51 PST
Created attachment 418142 [details]
Make Wasm/B3 a compile-time option, v11

v11,
and another fix for TV/iOS
Comment 18 Xan Lopez 2021-01-22 06:17:19 PST
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
Comment 19 Don Olmstead 2021-01-22 13:38:51 PST
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
Comment 20 Xan Lopez 2021-01-22 14:40:58 PST
(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.
Comment 21 EWS 2021-01-23 04:32:28 PST
Committed r271775: <https://trac.webkit.org/changeset/271775>

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