Implement GetByVal inline caching for 32-bit JITs

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

In https://bugs.webkit.org/show_bug.cgi?id=202767, I'm going to implement it for 64-bit. It should be easy to make it work on 32-bit JIT too.

[Caio Lima]

Created attachment 383912 [details]
WIP - Patch

It starts the implementation. It is only supporting Baseline IC generation, however the IC code generated is not working on 32-bits yet.

[Caio Lima]

Created attachment 383995 [details]
WIP - Patch

This is implementing GetByVal IC into 32-bits.

[Caio Lima]

Created attachment 383996 [details]
WIP - Patch

Oops, wrong patch.

[Caio Lima]

Created attachment 384078 [details]
Patch

[Caio Lima]

Created attachment 384290 [details]
Patch

[Saam Barati]

Comment on attachment 384290 [details]
Patch

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

Nice. r=me

> Source/JavaScriptCore/bytecode/StructureStubInfo.h:223
>          GPRReg baseTagGPR;

This isn’t your patch, but I wonder why this is needed. I think all users of StructureStubInfo branch around the base being a cell. But it might be worth looking at to verify and perhaps remove in the future.

> Source/JavaScriptCore/bytecode/StructureStubInfo.h:224
> +        union {

Why not put this in “u”?

[Caio Lima]

Comment on attachment 384290 [details]
Patch

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

Thank you very much for the review!

>> Source/JavaScriptCore/bytecode/StructureStubInfo.h:223
>>          GPRReg baseTagGPR;
> 
> This isn’t your patch, but I wonder why this is needed. I think all users of StructureStubInfo branch around the base being a cell. But it might be worth looking at to verify and perhaps remove in the future.

I think you are right here. I didn't check in a lot of places, but I do't remember a case where this is used, since all `JIT*InlineCacheGenerator` I saw was using `JSValueRegs::payloadOnly`. I created https://bugs.webkit.org/show_bug.cgi?id=204726 to care of it.

>> Source/JavaScriptCore/bytecode/StructureStubInfo.h:224
>> +        union {
> 
> Why not put this in “u”?

"u" is already being used by `thisPayloadGPR` and `propertyPayloadGPR`. If we would like to only have `u`, I think we can use `std::pair` or other structure, but I can't see how it would improve code quality.

[Caio Lima]

Created attachment 384573 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 384573 [details]
Patch

Clearing flags on attachment: 384573

Committed r252974: <https://trac.webkit.org/changeset/252974>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57548177>