| Summary: | speculateNeitherDoubleNorStringNorHeapBigInt should only have a single JSType branch | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||
| Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ews-watchlist, mark.lam, msaboff, rmorisset, saam, tzagallo, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Keith Miller
2021-07-21 07:19:01 PDT
Created attachment 433934 [details]
Patch
Comment on attachment 433934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433934&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:11792 > + DFG_TYPE_CHECK(regs, edge, ~(SpecString | SpecHeapBigInt), m_jit.branchIfType(regs.payloadGPR(), JSTypeRange { StringType, HeapBigIntType })); It seems like this should be SpecAnyString but maybe I was missing something? Comment on attachment 433934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433934&action=review >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:11792 >> + DFG_TYPE_CHECK(regs, edge, ~(SpecString | SpecHeapBigInt), m_jit.branchIfType(regs.payloadGPR(), JSTypeRange { StringType, HeapBigIntType })); > > It seems like this should be SpecAnyString but maybe I was missing something? Nvm, I confused myself, I was thinking that SpecString independent from SpecStringIdent and SpecStringVar... Comment on attachment 433934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433934&action=review r=me > Source/JavaScriptCore/ChangeLog:7 > + Maybe add some small comment here explaining your trick for people scanning through the Changelog. >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:11792 >> + DFG_TYPE_CHECK(regs, edge, ~(SpecString | SpecHeapBigInt), m_jit.branchIfType(regs.payloadGPR(), JSTypeRange { StringType, HeapBigIntType })); > > It seems like this should be SpecAnyString but maybe I was missing something? SpecAnyString does not exist, SpecString = SpecStringIdent | SpecStringVar is the union type. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:-19012 > - return m_out.equal( I think I would have kept this version of isType intact, and called it from the JSTypeRange version when range.last == range.first, but I'm ok with your approach even if I find it less intuitive. Comment on attachment 433934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433934&action=review >> Source/JavaScriptCore/ChangeLog:7 >> + > > Maybe add some small comment here explaining your trick for people scanning through the Changelog. Sure. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:-19012 >> - return m_out.equal( > > I think I would have kept this version of isType intact, and called it from the JSTypeRange version when range.last == range.first, but I'm ok with your approach even if I find it less intuitive. I just followed how the AssemblyHelpers version did it for consistency. I don't have strong feelings either way though. Created attachment 433942 [details]
Patch for landing
Committed r280149 (239846@main): <https://commits.webkit.org/239846@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433942 [details]. |