Bug 226621

Summary:

DFG should speculate on CompareStrictEq(@x, @x)

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer

Priority:

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