Bug 220412 - [JSC] Implement a B3::ValueRep replacement for wasm-llint
Summary: [JSC] Implement a B3::ValueRep 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-07 06:46 PST by Xan Lopez
Modified: 2021-01-14 14:48 PST (History)
9 users (show)

See Also:


Attachments
WIP patch. (39.33 KB, patch)
2021-01-07 06:49 PST, Xan Lopez
no flags Details | Formatted Diff | Diff
WIP patch. (22.35 KB, patch)
2021-01-12 07:01 PST, Xan Lopez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WasmValueLocation, v1 (26.51 KB, patch)
2021-01-14 08:29 PST, Xan Lopez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WasmValueLocation, v2 (28.14 KB, patch)
2021-01-14 08:35 PST, Xan Lopez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WasmValueLocation, v3 (28.26 KB, patch)
2021-01-14 08:41 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-07 06:46:12 PST
This is used unconditionally by WebAssembly (even at the interpreter level), so we need to move it somewhere else if we want a B3-less implementation. It's a self-contained class mostly tasked with representing how a value is laid out. See bug #220365.
Comment 1 Xan Lopez 2021-01-07 06:49:44 PST
Created attachment 417175 [details]
WIP patch.

WIP.

A couple things:
- There's a node type in DFG also named ValueRep. In some cases I had to add extra namespace information to avoid ambiguities.
- See the changed comment in ValueRep.h wrt ValueRecovery. It seems to me we could try to create a class that could be used by the wasm code using ValueRep, either based on ValueRecovery or merging bits from both. In any case probably moving stuff to bytecode/ first is a good idea.
Comment 2 Xan Lopez 2021-01-07 10:36:49 PST
After some talk today offline we'll try to come up with a class that WasmCallingConvention can use just for its needs, to remove the B3::ValueRep dependency on LLInt code. Possibly will need o translator to B3::ValueRep for the B3/Air generators in wasm/, but that should be simple? We'll see.
Comment 3 Xan Lopez 2021-01-12 07:01:43 PST
Created attachment 417453 [details]
WIP patch.

New WIP patch.

This just takes B3::ValueRep, removes everything that Wasm/LLInt does not use (most of it), renames it to WasmValueLocation and uses it where needed. I added a method to convert a WasmValueLocation to a B3::ValueRep and use it in the B3/Air generators, which actually do need to use the full B3::ValueRep.
Comment 4 Saam Barati 2021-01-12 10:59:19 PST
Comment on attachment 417453 [details]
WIP patch.

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

> Source/JavaScriptCore/wasm/WasmValueLocation.h:65
> +    static ValueLocation stack(intptr_t offsetFromFP)

do we really need this and stackArgument?
Comment 5 Xan Lopez 2021-01-13 01:43:49 PST
(In reply to Saam Barati from comment #4)
> Comment on attachment 417453 [details]
> WIP patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417453&action=review
> 
> > Source/JavaScriptCore/wasm/WasmValueLocation.h:65
> > +    static ValueLocation stack(intptr_t offsetFromFP)
> 
> do we really need this and stackArgument?

All three 'kinds' left in the class (reg, stack, stackArgument) are used in WasmCallingConvention (see marshallLocationImpl), if that's what you are asking. If you mean whether the class *really* needs that, not sure. The code seems sensible enough, but it's the first time I work on it.
Comment 6 Radar WebKit Bug Importer 2021-01-14 06:47:13 PST
<rdar://problem/73196482>
Comment 7 Xan Lopez 2021-01-14 08:29:57 PST
Created attachment 417624 [details]
WasmValueLocation, v1

v1, add the Mac build bits and fill in the ChangeLog.
Comment 8 Xan Lopez 2021-01-14 08:35:21 PST
Created attachment 417625 [details]
WasmValueLocation, v2

v2, try again with the Mac build.
Comment 9 Xan Lopez 2021-01-14 08:41:35 PST
Created attachment 417626 [details]
WasmValueLocation, v3

v3, try to fix the Watch build.
Comment 10 Yusuke Suzuki 2021-01-14 14:44:19 PST
Comment on attachment 417626 [details]
WasmValueLocation, v3

r=me
Comment 11 EWS 2021-01-14 14:48:47 PST
Committed r271500: <https://trac.webkit.org/changeset/271500>

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