Bug 208207

Summary: [JSC] Support delete by val/id IC on 32-bits
Product: WebKit Reporter: Caio Lima <ticaiolima>
Component: JavaScriptCoreAssignee: Caio Lima <ticaiolima>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, jsc32, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP - Patch
none
Patch
none
Patch none

Description Caio Lima 2020-02-25 10:04:38 PST 
We are adding IC to delete_by_val and delete_by_id on https://bugs.webkit.org/show_bug.cgi?id=207522, but it doesn't include support for 32-bits architectures.
Comment 1 Caio Lima 2020-05-06 15:12:55 PDT 
Created attachment 398672 [details]
WIP - Patch
Comment 2 Caio Lima 2020-05-06 15:13:23 PDT 
Comment on attachment 398672 [details]
WIP - Patch

Let's check EWS.
Comment 3 Caio Lima 2020-05-11 11:45:57 PDT 
Created attachment 399036 [details]
Patch
Comment 4 Saam Barati 2020-05-11 18:34:40 PDT 
Comment on attachment 399036 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1175
> +        JSValueOperand key(this, node->child2(), ManualOperandSpeculation);

do we ever pick anything besides UntypedUse? Where is our speculation?
Comment 5 Caio Lima 2020-05-12 04:16:11 PDT 
Comment on attachment 399036 [details]
Patch

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

Thank you very much for the comments! I'm answering questions below.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1175
>> +        JSValueOperand key(this, node->child2(), ManualOperandSpeculation);
> 
> do we ever pick anything besides UntypedUse? Where is our speculation?

IIUC, we have rules to fix up to CellUse from fix up phase. The speculation is just above at line 1172.
Comment 6 Saam Barati 2020-05-12 11:29:21 PDT 
Comment on attachment 399036 [details]
Patch

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

r=me

>>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1175
>>> +        JSValueOperand key(this, node->child2(), ManualOperandSpeculation);
>> 
>> do we ever pick anything besides UntypedUse? Where is our speculation?
> 
> IIUC, we have rules to fix up to CellUse from fix up phase. The speculation is just above at line 1172.

I see. This style is weird, we typically speculate after such a LOC. (I understand you're just refactoring the code here)
Comment 7 Caio Lima 2020-05-12 12:26:54 PDT 
Created attachment 399153 [details]
Patch
Comment 8 Caio Lima 2020-05-12 12:34:43 PDT 
Comment on attachment 399036 [details]
Patch

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

Thank you very much for the review

>>>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1175
>>>> +        JSValueOperand key(this, node->child2(), ManualOperandSpeculation);
>>> 
>>> do we ever pick anything besides UntypedUse? Where is our speculation?
>> 
>> IIUC, we have rules to fix up to CellUse from fix up phase. The speculation is just above at line 1172.
> 
> I see. This style is weird, we typically speculate after such a LOC. (I understand you're just refactoring the code here)

I agree. I fixed it.
Comment 9 EWS 2020-05-13 05:16:40 PDT 
Committed r261610: <https://trac.webkit.org/changeset/261610>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399153 [details].
Comment 10 Radar WebKit Bug Importer 2020-05-13 05:17:13 PDT 
<rdar://problem/63180603>