Bug 69321 – JITCodeGenerator should no longer have code that tries too hard to be both speculative and non-speculative

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

[Filip Pizlo]

JITCodeGenerator used to be a superclass for NonSpeculativeJIT and SpeculativeJIT.  Now the NonSpeculativeJIT is gone.  There are some warts from this:

- JITCodeGenerator has a m_isSpeculative field.  This field is now always true.

- JITCodeGenerator has a method to emit a speculation check, if we're in speculative mode.  Code that needs speculation checks should probably just be moved to SpeculativeJIT.

- JITCodeGenerator has an emitBranch method that tries to unify both speculative and non-speculative handling of branches.  Now it should probably just be speculative.

[Filip Pizlo]

Created attachment 109585 [details]
the patch

[Oliver Hunt]

Comment on attachment 109585 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109585&action=review

While i realize this patch is only trying to simplify the emitBranch logic, it seems sad that we don't emit code for object checks, etc inline when the profiles tell us we're looking at objects.

r- though because the 64bit version of emitBranch has weird control flow layout

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:487
> +    // FIXME: Add fast cases for known Boolean!

Boolean implies boxed boolean to me -- do you mean boxed or unboxed here?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:496
> +    use(node.child1());

What does this do? (unrelated to this particular patch, i just don't think i've seen use() before)

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:502
> +    JITCompiler::Jump fastPath = m_jit.branch32(JITCompiler::Equal, valueTagGPR, JITCompiler::TrustedImm32(JSValue::Int32Tag));
> +    JITCompiler::Jump slowPath = m_jit.branch32(JITCompiler::NotEqual, valueTagGPR, JITCompiler::TrustedImm32(JSValue::BooleanTag));

It would be nice if we could rework our encoding to make this work with a single branch, but again unrelated to this patch

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:593
> +    if (isKnownBoolean(node.child1())) {

no numeric fast case?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:621
> +        if (predictBoolean) {
> +            addBranch(m_jit.branchPtr(MacroAssembler::Equal, valueGPR, MacroAssembler::ImmPtr(JSValue::encode(jsBoolean(false)))), notTaken);
> +            addBranch(m_jit.branchPtr(MacroAssembler::Equal, valueGPR, MacroAssembler::ImmPtr(JSValue::encode(jsBoolean(true)))), taken);
> +        }
> +        
> +        if (predictBoolean) {
> +            speculationCheck(m_jit.jump());
> +            value.use();

Why are these in two separate blocks

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:626
> +            if (!predictBoolean) {

This branch is always taken

[Filip Pizlo]

(In reply to comment #2)
> (From update of attachment 109585 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109585&action=review
> 
> While i realize this patch is only trying to simplify the emitBranch logic, it seems sad that we don't emit code for object checks, etc inline when the profiles tell us we're looking at objects.

Actually, it's trying to eliminate speculative code from the DFGJITCodeGenerator, and eliminate code in DFGJITCodeGenerator that tries to be generic generic over m_isSpeculative.  emitBranch() was simply moved, almost entirely untouched, from DFGJITCodeGenerator into DFGSpeculativeJIT.

> 
> r- though because the 64bit version of emitBranch has weird control flow layout
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:487
> > +    // FIXME: Add fast cases for known Boolean!
> 
> Boolean implies boxed boolean to me -- do you mean boxed or unboxed here?

Not my comment, it was already there.

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:496
> > +    use(node.child1());
> 
> What does this do? (unrelated to this particular patch, i just don't think i've seen use() before)

I didn't add it in this patch, it was already there.

It simply marks to the reg alloc that the value associated with this node does not have to be kept alive if we spill/fill.

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:502
> > +    JITCompiler::Jump fastPath = m_jit.branch32(JITCompiler::Equal, valueTagGPR, JITCompiler::TrustedImm32(JSValue::Int32Tag));
> > +    JITCompiler::Jump slowPath = m_jit.branch32(JITCompiler::NotEqual, valueTagGPR, JITCompiler::TrustedImm32(JSValue::BooleanTag));
> 
> It would be nice if we could rework our encoding to make this work with a single branch, but again unrelated to this patch

That's not my code; I think our friend at Intel wrote that.

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:593
> > +    if (isKnownBoolean(node.child1())) {
> 
> no numeric fast case?

There has never been one in this code.  I have a separate bug for that: https://bugs.webkit.org/show_bug.cgi?id=69322

My plan was to put that up for review once emitBranch() was moved to SpeculativeJIT.

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:621
> > +        if (predictBoolean) {
> > +            addBranch(m_jit.branchPtr(MacroAssembler::Equal, valueGPR, MacroAssembler::ImmPtr(JSValue::encode(jsBoolean(false)))), notTaken);
> > +            addBranch(m_jit.branchPtr(MacroAssembler::Equal, valueGPR, MacroAssembler::ImmPtr(JSValue::encode(jsBoolean(true)))), taken);
> > +        }
> > +        
> > +        if (predictBoolean) {
> > +            speculationCheck(m_jit.jump());
> > +            value.use();
> 
> Why are these in two separate blocks

That's a screw-up.  Previously the second block tested for m_isSpeculative.  Now it doesn't, so I guess they should be merged and….

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:626
> > +            if (!predictBoolean) {
> 
> This branch is always taken

This should be killed.

Bottom line, I'm happy to simply just do this as one patch (see https://bugs.webkit.org/show_bug.cgi?id=69322), but I wanted to have a separate patch that gets rid of:

JITCodeGenerator::m_isSpeculative  (it's always true)
JITCodeGenerator::speculationCheck  (it's a horrible hack from the days when the DFG had two JITs)

and to do that I have to move emitBranch() to SpeculativeJIT, since that's the most egregious use of the two above things, which only existed because previously emitBranch() had to be in JITCodeGenerator to avoid code duplication.

[Filip Pizlo]

Created attachment 109676 [details]
the patch - address some of review

The goal of this patch is to remove m_isSpeculative and speculationCheck() from JITCodeGenerator, since neither of those members belong there now that we don't have a non-speculative DFG JIT.  To do that, it moves emitBranch() to SpeculativeJIT, and does not change it other than obvious clean-ups (like when you remove m_isSpeculative and assume that it's always true then some control flow changes).

[Gavin Barraclough]

I'm a little confused, is speculationCheck not used in other places? assuming I'm missing something, r+.

[Filip Pizlo]

(In reply to comment #5)
> I'm a little confused, is speculationCheck not used in other places? assuming I'm missing something, r+.

I'm referring to JITCodeGenerator::speculationCheck() which did something like:

void JITCodeGenerator::speculationCheck(...)
{
   ASSERT(m_isSpeculative);
   static_cast<SpeculativeJIT*>(this)->speculationCheck(...);
}

[Filip Pizlo]

Landed in r96661.