Bug 226621

Summary: DFG should speculate on CompareStrictEq(@x, @x)
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: All   
OS: All   
Attachments:
Description Flags
the patch mark.lam: review+

Filip Pizlo
Reported 2021-06-03 20:45:00 PDT
...
Attachments
the patch (10.80 KB, patch)
2021-06-03 20:48 PDT, Filip Pizlo 		mark.lam: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 2021-06-03 20:48:03 PDT
Created attachment 430535 [details] the patch Not yet ready for review - still need to test it.
Mark Lam
Comment 2 2021-06-04 09:21:37 PDT
Comment on attachment 430535 [details] the patch r=me
Filip Pizlo
Comment 3 2021-06-04 09:33:22 PDT
Landed in https://trac.webkit.org/changeset/278465/webkit
Radar WebKit Bug Importer
Comment 4 2021-06-04 09:34:16 PDT
<rdar://problem/78872738>
Saam Barati
Comment 5 2021-06-04 09:50:48 PDT
Comment on attachment 430535 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=430535&action=review LGTM too, one comment > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:11387 > + JITCompiler::Jump done = m_jit.branchIfInt32(regs); nit: we always do this branch, even if we're proven not to be a double. Maybe let's read AI state before doing this > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:18287 > + m_out.branch(isInt32(value, provenType(edge)), unsure(continuation), unsure(isNotInt32)); ditto
Filip Pizlo
Comment 6 2021-06-04 10:17:47 PDT
(In reply to Saam Barati from comment #5) > Comment on attachment 430535 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=430535&action=review > > LGTM too, one comment > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:11387 > > + JITCompiler::Jump done = m_jit.branchIfInt32(regs); > > nit: we always do this branch, even if we're proven not to be a double. If we're proven not to be a double then this entire check would have been eliminated. > > Maybe let's read AI state before doing this > > > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:18287 > > + m_out.branch(isInt32(value, provenType(edge)), unsure(continuation), unsure(isNotInt32)); > > ditto This actually reads AI state (that's the provenType(edge) thing). I think I'll leave it as is for now. We've got bigger fish to fry in this area.
Saam Barati
Comment 7 2021-06-04 14:22:13 PDT
(In reply to Filip Pizlo from comment #6) > (In reply to Saam Barati from comment #5) > > Comment on attachment 430535 [details] > > the patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=430535&action=review > > > > LGTM too, one comment > > > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:11387 > > > + JITCompiler::Jump done = m_jit.branchIfInt32(regs); > > > > nit: we always do this branch, even if we're proven not to be a double. > > If we're proven not to be a double then this entire check would have been > eliminated. > > > > > Maybe let's read AI state before doing this > > > > > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:18287 > > > + m_out.branch(isInt32(value, provenType(edge)), unsure(continuation), unsure(isNotInt32)); > > > > ditto > > This actually reads AI state (that's the provenType(edge) thing). > > I think I'll leave it as is for now. We've got bigger fish to fry in this > area. We discussed over slack. Phil landed this: https://trac.webkit.org/changeset/278476/webkit
Note You need to log in before you can comment on or make changes to this bug.