Bug 226621 – DFG should speculate on CompareStrictEq(@x, @x)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

...

[Filip Pizlo]

Created attachment 430535 [details]
the patch

Not yet ready for review - still need to test it.

[Mark Lam]

Comment on attachment 430535 [details]
the patch

r=me

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/278465/webkit

[Radar WebKit Bug Importer]

<rdar://problem/78872738>

[Saam Barati]

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]

(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]

(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