Bug 159700 – DFG should really support jneq_ptr

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 283485 [details]
the patch

[Keith Miller]

Comment on attachment 283485 [details]
the patch

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

r=me with question.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8268
> +    m_jit.boxBooleanPayload(false, resultGPR);
> +    JITCompiler::JumpList notEqual = m_jit.branchIfNotEqual(regs, node->cellOperand()->value());
> +    m_jit.boxBooleanPayload(true, resultGPR);
> +    notEqual.link(&m_jit);

Is this more efficient than doing:

JITCompiler::JumpList notEqual = m_jit.branchIfNotEqual(regs, node->cellOperand()->value());
m_jit.boxBooleanPayload(true, resultGPR);
JITCompiler::Jump done = m_jit.jump();
notEqual.link(&m_jit);
m_jit.boxBooleanPayload(false, resultGPR);
done.link(&m_jit);

[Filip Pizlo]

(In reply to comment #2)
> Comment on attachment 283485 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283485&action=review
> 
> r=me with question.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8268
> > +    m_jit.boxBooleanPayload(false, resultGPR);
> > +    JITCompiler::JumpList notEqual = m_jit.branchIfNotEqual(regs, node->cellOperand()->value());
> > +    m_jit.boxBooleanPayload(true, resultGPR);
> > +    notEqual.link(&m_jit);
> 
> Is this more efficient than doing:
> 
> JITCompiler::JumpList notEqual = m_jit.branchIfNotEqual(regs,
> node->cellOperand()->value());
> m_jit.boxBooleanPayload(true, resultGPR);
> JITCompiler::Jump done = m_jit.jump();
> notEqual.link(&m_jit);
> m_jit.boxBooleanPayload(false, resultGPR);
> done.link(&m_jit);

I don't know!  I think that my version is better because:

- Moving zero into a register is basically free.
- Your version will do two branches in the worst case, while mine will do one.  One of your branches is a jump, which is cheap, but probably worse than zeroing a register.

My version also involves less code.  I like to err on the side of small code when everything else is up in the air.

[Filip Pizlo]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 283485 [details]
> > the patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=283485&action=review
> > 
> > r=me with question.
> > 
> > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:8268
> > > +    m_jit.boxBooleanPayload(false, resultGPR);
> > > +    JITCompiler::JumpList notEqual = m_jit.branchIfNotEqual(regs, node->cellOperand()->value());
> > > +    m_jit.boxBooleanPayload(true, resultGPR);
> > > +    notEqual.link(&m_jit);
> > 
> > Is this more efficient than doing:
> > 
> > JITCompiler::JumpList notEqual = m_jit.branchIfNotEqual(regs,
> > node->cellOperand()->value());
> > m_jit.boxBooleanPayload(true, resultGPR);
> > JITCompiler::Jump done = m_jit.jump();
> > notEqual.link(&m_jit);
> > m_jit.boxBooleanPayload(false, resultGPR);
> > done.link(&m_jit);
> 
> I don't know!  I think that my version is better because:
> 
> - Moving zero into a register is basically free.
> - Your version will do two branches in the worst case, while mine will do
> one.  One of your branches is a jump, which is cheap, but probably worse
> than zeroing a register.
> 
> My version also involves less code.  I like to err on the side of small code
> when everything else is up in the air.

Brainfart: I'm not moving zero into a register.  I guess the question is: do we care about this code being fast or small?  I like small a bit better, since if it really needs to be fast, it'll get to FTL.  DFG is for functions that don't run that much, so icache matters more.

[Filip Pizlo]

Created attachment 283486 [details]
patch for landing

[Filip Pizlo]

Created attachment 283549 [details]
performance

Looks pretty good!

[Filip Pizlo]

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