Bug 220585 - [JSC] Implement a B3::Compilation replacement for wasm-llint
Summary: [JSC] Implement a B3::Compilation replacement for wasm-llint
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:
Blocks: 220365
  Show dependency treegraph
 
Reported: 2021-01-13 03:57 PST by Xan Lopez
Modified: 2021-01-18 16:25 PST (History)
13 users (show)

See Also:


Attachments
WIP. (40.04 KB, patch)
2021-01-15 08:55 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
WIP. (40.89 KB, patch)
2021-01-15 09:13 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
Move B3::Compilation to jit/, v1 (58.43 KB, patch)
2021-01-16 02:24 PST, Xan Lopez
ysuzuki: review+
ysuzuki: commit-queue-
Details | Formatted Diff | Diff
Move B3::Compilation to jit/, v2 (58.94 KB, patch)
2021-01-18 03:26 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-13 03:57:46 PST
This is used by WasmLLIntPlan, which prevents us from compiling Wasm without B3 support.
Comment 1 Xan Lopez 2021-01-13 04:05:50 PST
B3::Compilation is basically:
- A MacroAssemblerCodeRef.
- A vector of opaque "compilation byproducts". This, incidentally, is never used by WasmLLInt (it's always set to null).

I see a few options:
- We could move this to jit/. This would remove the dependency from B3, but would still be a sort of layering violation, since it would make LLInt code depend on jit/ (which is still the case in other places, for now).
- We could move this even further down the stack?
- We could make WasmLLInt use something else when B3 is disabled, and keep the code as-is when B3 is enabled.

I suppose the cleanest solutions in terms of the end result would be either #2 or #1, but comments are welcome.
Comment 2 Xan Lopez 2021-01-15 08:55:09 PST
Created attachment 417704 [details]
WIP.

WIP patch.

Having a closer look at the code it seems to me there's so much stuff depending on JIT atm that there's no point in trying to put B3Compilation elsewhere. Since it fits nicely there, let's start with that. WIP patch attached, 100% mechanical (I've left the JSC namespace in front of classes so I can grep easily, but it could be removed in the final patch).
Comment 3 Xan Lopez 2021-01-15 09:13:01 PST
Created attachment 417707 [details]
WIP.
Comment 4 Xan Lopez 2021-01-16 02:24:34 PST
Created attachment 417764 [details]
Move B3::Compilation to jit/, v1

Try the build bots, v1
Comment 5 Yusuke Suzuki 2021-01-16 14:47:45 PST
Comment on attachment 417764 [details]
Move B3::Compilation to jit/, v1

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

r=me

> Source/JavaScriptCore/b3/air/testair.cpp:88
> +std::unique_ptr<JSC::Compilation> compile(B3::Procedure& proc)

JSC:: of JSC::Compilation is not necessary.

> Source/JavaScriptCore/b3/air/testair.cpp:95
> +    return makeUnique<JSC::Compilation>(

JSC:: is not necessary.

> Source/JavaScriptCore/b3/air/testair.cpp:100
> +T invoke(const JSC::Compilation& code, Arguments... arguments)

Ditto.

> Source/JavaScriptCore/ftl/FTLJITCode.h:55
> +    void initializeB3Byproducts(std::unique_ptr<JSC::OpaqueByproducts>);

JSC:: of JSC::OpaqueByproducts is not necessary.

> Source/JavaScriptCore/ftl/FTLJITCode.h:80
> +    std::unique_ptr<JSC::OpaqueByproducts> m_b3Byproducts;

Ditto.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.h:50
> +    std::unique_ptr<JSC::OpaqueByproducts> wasmEntrypointByproducts;

Ditto

> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:113
> +    function->entrypoint.compilation = makeUnique<JSC::Compilation>(

Ditto

> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:229
> +            m_wasmInternalFunctions[functionIndex]->entrypoint.compilation = makeUnique<JSC::Compilation>(

Ditto.

> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:241
> +            embedderToWasmInternalFunction->entrypoint.compilation = makeUnique<JSC::Compilation>(

Ditto

> Source/JavaScriptCore/wasm/WasmFormat.h:341
> +    std::unique_ptr<JSC::Compilation> compilation;

Ditto.

> Source/JavaScriptCore/wasm/WasmLLIntPlan.cpp:155
> +            function->entrypoint.compilation = makeUnique<JSC::Compilation>(

Ditto.

> Source/JavaScriptCore/wasm/WasmOMGForOSREntryPlan.cpp:92
> +    omgEntrypoint.compilation = makeUnique<JSC::Compilation>(

Ditto

> Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:89
> +    omgEntrypoint.compilation = makeUnique<JSC::Compilation>(

Ditto.
Comment 6 Xan Lopez 2021-01-18 03:26:16 PST
Created attachment 417827 [details]
Move B3::Compilation to jit/, v2

v2, remove unnecessary JSC:: namespace (which also found a redundant and mistaken B3::Compilation declaration left).
Comment 7 EWS 2021-01-18 16:24:57 PST
Committed r271594: <https://trac.webkit.org/changeset/271594>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417827 [details].
Comment 8 Radar WebKit Bug Importer 2021-01-18 16:25:26 PST
<rdar://problem/73332409>