Bug 232265 – Refactoring and PutByVal cleanup

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mikhail R. Gadelha]

Refactoring and PutByVal cleanup

[Mikhail R. Gadelha]

Created attachment 442416 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 442516 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 442524 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/84897622>

[Saam Barati]

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?

[Mikhail R. Gadelha]

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.

[Mikhail R. Gadelha]

Created attachment 443311 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 443410 [details]
Patch

[Saam Barati]

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.

[Mikhail R. Gadelha]

Created attachment 443565 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 443678 [details]
Patch

[EWS]

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].

[Yusuke Suzuki]

Caused the regression, tracking in https://bugs.webkit.org/show_bug.cgi?id=233147