Bug 234051

Summary: Support WasmAddress in B3 CSE
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
none
WIP
none
patch
fpizlo: review+
patch for landing
none
patch for landing
none
patch for landing none

Description Saam Barati 2021-12-08 18:19:56 PST 
...
Comment 1 Saam Barati 2021-12-09 11:41:32 PST 
Created attachment 446579 [details]
WIP
Comment 2 Saam Barati 2021-12-10 14:59:24 PST 
Created attachment 446822 [details]
WIP
Comment 3 Radar WebKit Bug Importer 2021-12-15 18:20:20 PST 
<rdar://problem/86552957>
Comment 4 Saam Barati 2021-12-16 17:01:33 PST 
Created attachment 447398 [details]
patch
Comment 5 Yusuke Suzuki 2021-12-16 17:04:57 PST 
Comment on attachment 447398 [details]
patch

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

r=me too.

> Source/JavaScriptCore/b3/B3EliminateCommonSubexpressions.cpp:198
> +                if (WasmAddressValue* wasmAddress = value->as<WasmAddressValue>())
> +                    data.m_wasmAddressesAtTail.add(wasmAddress->child(0), wasmAddress);

Let's avoid adding this if data.writesPinned is already true.

> Source/JavaScriptCore/b3/B3EliminateCommonSubexpressions.cpp:261
> +        if (WasmAddressValue* wasmAddress = m_value->as<WasmAddressValue>()) {
> +            processWasmAddressValue(wasmAddress);
> +            return;
> +        }

Ditto.
Comment 6 Yusuke Suzuki 2021-12-16 17:06:59 PST 
Comment on attachment 447398 [details]
patch

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

>> Source/JavaScriptCore/b3/B3EliminateCommonSubexpressions.cpp:198
>> +                    data.m_wasmAddressesAtTail.add(wasmAddress->child(0), wasmAddress);
> 
> Let's avoid adding this if data.writesPinned is already true.

Ah, I misunderstood. This is tracking wasm addresses which are still valid.
Comment 7 Saam Barati 2021-12-16 17:07:30 PST 
Comment on attachment 447398 [details]
patch

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

>> Source/JavaScriptCore/b3/B3EliminateCommonSubexpressions.cpp:198
>> +                    data.m_wasmAddressesAtTail.add(wasmAddress->child(0), wasmAddress);
> 
> Let's avoid adding this if data.writesPinned is already true.

This is still useful because you could still CSE with values that come *after* writes pinned.
Comment 8 Saam Barati 2021-12-16 17:09:45 PST 
Created attachment 447399 [details]
patch for landing
Comment 9 Saam Barati 2021-12-16 17:11:20 PST 
Created attachment 447400 [details]
patch for landing
Comment 10 Saam Barati 2021-12-16 17:12:14 PST 
Created attachment 447401 [details]
patch for landing
Comment 11 EWS 2021-12-17 13:07:04 PST 
Committed r287203 (245370@main): <https://commits.webkit.org/245370@main>

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