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, sbarati, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: All   
OS: All   
Attachments:
Description Flags
the patch mark.lam: review+

Description Filip Pizlo 2021-06-03 20:45:00 PDT
...
Comment 1 Filip Pizlo 2021-06-03 20:48:03 PDT
Created attachment 430535 [details]
the patch

Not yet ready for review - still need to test it.
Comment 2 Mark Lam 2021-06-04 09:21:37 PDT
Comment on attachment 430535 [details]
the patch

r=me
Comment 3 Filip Pizlo 2021-06-04 09:33:22 PDT
Landed in https://trac.webkit.org/changeset/278465/webkit
Comment 4 Radar WebKit Bug Importer 2021-06-04 09:34:16 PDT
<rdar://problem/78872738>
Comment 5 Saam Barati 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
Comment 6 Filip Pizlo 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.
Comment 7 Saam Barati 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