Bug 232265 - Refactoring and PutByVal cleanup
Summary: Refactoring and PutByVal cleanup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 233147
  Show dependency tree / graph
 
Reported: 2021-10-25 13:47 PDT by Mikhail R. Gadelha
Modified: 2021-11-15 14:31 PST (History)
9 users (show)

See Also:

Attachments
Patch (100.17 KB, patch)
2021-10-25 14:17 PDT, Mikhail R. Gadelha 		no flags Details | Formatted Diff | Diff
Patch (99.68 KB, patch)
2021-10-26 12:24 PDT, Mikhail R. Gadelha 		no flags Details | Formatted Diff | Diff
Patch (99.97 KB, patch)
2021-10-26 13:37 PDT, Mikhail R. Gadelha 		no flags Details | Formatted Diff | Diff
Patch (88.89 KB, patch)
2021-11-04 09:19 PDT, Mikhail R. Gadelha 		no flags Details | Formatted Diff | Diff
Patch (88.86 KB, patch)
2021-11-05 09:50 PDT, Mikhail R. Gadelha 		no flags Details | Formatted Diff | Diff
Patch (88.89 KB, patch)
2021-11-08 10:29 PST, Mikhail R. Gadelha 		no flags Details | Formatted Diff | Diff
Patch (90.31 KB, patch)
2021-11-09 04:19 PST, Mikhail R. Gadelha 		no flags Details | Formatted Diff | Diff
Show Obsolete (6) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail R. Gadelha 2021-10-25 13:47:05 PDT 
Refactoring and PutByVal cleanup
Comment 1 Mikhail R. Gadelha 2021-10-25 14:17:07 PDT 
Created attachment 442416 [details]
Patch
Comment 2 Mikhail R. Gadelha 2021-10-26 12:24:41 PDT 
Created attachment 442516 [details]
Patch
Comment 3 Mikhail R. Gadelha 2021-10-26 13:37:22 PDT 
Created attachment 442524 [details]
Patch
Comment 4 Radar WebKit Bug Importer 2021-11-01 13:48:44 PDT 
<rdar://problem/84897622>
Comment 5 Saam Barati 2021-11-03 11:33:26 PDT 
Comment on attachment 442524 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2513
> +#if USE(JSVALUE32_64)
> +        m_jit.store32(valueTagReg, MacroAssembler::BaseIndex(storageReg, propertyReg, MacroAssembler::TimesEight, OBJECT_OFFSETOF(JSValue, u.asBits.tag)));
> +        m_jit.store32(valuePayloadReg, MacroAssembler::BaseIndex(storageReg, propertyReg, MacroAssembler::TimesEight, OBJECT_OFFSETOF(JSValue, u.asBits.payload)));
> +#else
> +        m_jit.store64(valueReg, MacroAssembler::BaseIndex(storageReg, propertyReg, MacroAssembler::TimesEight));
> +#endif

Is it worth making this code JSValueRegs based?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2547
> +#if USE(JSVALUE32_64)
> +    m_jit.store32(valueTagReg, MacroAssembler::BaseIndex(storageReg, propertyReg, MacroAssembler::TimesEight, OBJECT_OFFSETOF(JSValue, u.asBits.tag)));
> +    m_jit.store32(valuePayloadReg, MacroAssembler::BaseIndex(storageReg, propertyReg, MacroAssembler::TimesEight, OBJECT_OFFSETOF(JSValue, u.asBits.payload)));
> +#else
> +    m_jit.store64(valueReg, MacroAssembler::BaseIndex(storageReg, propertyReg, MacroAssembler::TimesEight));
> +#endif

ditto

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:3555
> +        compilePutByVal(node);

why? This seems to be just moving code around within the same file. Is the goal just to have compilePutByVal functions on both 32-bit and 64-bit, even though they don't share an implementation?
Comment 6 Mikhail R. Gadelha 2021-11-03 13:34:56 PDT 
Comment on attachment 442524 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2513
>> +#endif
> 
> Is it worth making this code JSValueRegs based?

OK

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2547
>> +#endif
> 
> ditto

OK

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:3555
>> +        compilePutByVal(node);
> 
> why? This seems to be just moving code around within the same file. Is the goal just to have compilePutByVal functions on both 32-bit and 64-bit, even though they don't share an implementation?

Shipping a shared implementation was my initial plan but I thought the patch was already too long. I'll update it with the shared implementation.
Comment 7 Mikhail R. Gadelha 2021-11-04 09:19:26 PDT 
Created attachment 443311 [details]
Patch
Comment 8 Mikhail R. Gadelha 2021-11-05 09:50:27 PDT 
Created attachment 443410 [details]
Patch
Comment 9 Saam Barati 2021-11-08 09:10:32 PST 
Comment on attachment 443410 [details]
Patch

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

Nice. r=me

> Source/JavaScriptCore/ChangeLog:22
> +        6. Removed a lot of whitespace.

We don't typically do this. Although the style says not to leave trailing whitespace, we usually don't go back and remove trailing whitespace. I think the main motivation is to make it easier to `blame` code.
Comment 10 Mikhail R. Gadelha 2021-11-08 10:29:39 PST 
Created attachment 443565 [details]
Patch
Comment 11 Mikhail R. Gadelha 2021-11-09 04:19:33 PST 
Created attachment 443678 [details]
Patch
Comment 12 EWS 2021-11-09 13:33:42 PST 
Committed r285530 (244047@main): <https://commits.webkit.org/244047@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443678 [details].
Comment 13 Yusuke Suzuki 2021-11-15 14:31:07 PST 
Caused the regression, tracking in https://bugs.webkit.org/show_bug.cgi?id=233147