RESOLVED FIXED 204082
Implement GetByVal inline caching for 32-bit JITs 
https://bugs.webkit.org/show_bug.cgi?id=204082
Summary Implement GetByVal inline caching for 32-bit JITs
Saam Barati
Reported 2019-11-11 13:27:44 PST
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.
Attachments
WIP - Patch (10.76 KB, patch)
2019-11-19 15:09 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
WIP - Patch (12.75 KB, patch)
2019-11-20 15:34 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
WIP - Patch (16.14 KB, patch)
2019-11-20 15:35 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (22.86 KB, patch)
2019-11-21 11:49 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (25.41 KB, patch)
2019-11-25 04:51 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Patch (25.63 KB, patch)
2019-12-01 08:02 PST, Caio Lima 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Caio Lima
Comment 1 2019-11-19 15:09:54 PST
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
Comment 2 2019-11-20 15:34:11 PST
Created attachment 383995 [details] WIP - Patch This is implementing GetByVal IC into 32-bits.
Caio Lima
Comment 3 2019-11-20 15:35:17 PST
Created attachment 383996 [details] WIP - Patch Oops, wrong patch.
Caio Lima
Comment 4 2019-11-21 11:49:33 PST
Created attachment 384078 [details] Patch
Caio Lima
Comment 5 2019-11-25 04:51:40 PST
Created attachment 384290 [details] Patch
Saam Barati
Comment 6 2019-11-30 16:36:39 PST
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 7 2019-12-01 07:16:03 PST
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
Comment 8 2019-12-01 08:02:58 PST
Created attachment 384573 [details] Patch
WebKit Commit Bot
Comment 9 2019-12-01 18:54:19 PST
Comment on attachment 384573 [details] Patch Clearing flags on attachment: 384573 Committed r252974: <https://trac.webkit.org/changeset/252974>
WebKit Commit Bot
Comment 10 2019-12-01 18:54:21 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 11 2019-12-01 18:55:19 PST
<rdar://problem/57548177>
Note You need to log in before you can comment on or make changes to this bug.