Bug 151266 – Use the JITAddGenerator snippet in the DFG.

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

[Mark Lam]

Patch coming.

[Mark Lam]

Created attachment 265511 [details]
the patch.

[Mark Lam]

Created attachment 265512 [details]
x86_64 benchmark result.

[Mark Lam]

Created attachment 265513 [details]
x86 benchmark result.

[Mark Lam]

The benchmark runs show that perf is neutral.  Any changes are due to noise.  when I rerun the noise component individually, the diff disappears.

[Geoffrey Garen]

Comment on attachment 265511 [details]
the patch.

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

> Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:-502
> -    ASSERT(isType(SpecHeapTop));

Can't you still assert byte code top?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2845
> +        // Let's swap the operands.
> +        ResultType tempType = rightType;
> +        rightType = leftType;
> +        leftType = tempType;

Instead of saying "let's swap" in a comment, you can call std::swap.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2877
> +
> +    if (leftIsConstInt32) {
> +        // Since we may be dealing with non-numeric operands (which may not be commutative)
> +        // in the slow path, we need to restore the left right order.
> +        rightRegs = right.jsValueRegs();
> +        leftRegs = resultRegs;
> +        int64_t leftConst = node->child1()->asInt32();
> +        m_jit.moveValue(JSValue(leftConst), leftRegs);

Can we avoid the swapping concern by simply building into JITAddGenerator the ability to swap left and right internally if left is constant?

In addition to reducing code at the call sites, that seems like a better separation of concerns. JITAddGenerator's client does not know that swapping is appropriate since it does not know whether all of JITAddGenerator's fast path operations are commutative. Only JITAddGenerator can know that.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2587
> +#if USE(JSVALUE64)
> +        if (m_gprOrInvalid != InvalidGPRReg)
> +            m_jit->unlock(m_gprOrInvalid);
> +#elif USE(JSVALUE32_64)
> +        if (m_isDouble && m_register.fpr != InvalidFPRReg)
> +            m_jit->unlock(m_register.fpr);
> +        else if (m_register.pair.tagGPR != InvalidGPRReg && m_register.pair.payloadGPR != InvalidGPRReg) {
> +            m_jit->unlock(m_register.pair.tagGPR);
> +            m_jit->unlock(m_register.pair.payloadGPR);
> +        }
> +#endif

Is there a condition where we fill a register and then set it to not needed? It looks like we only set not needed when we choose not to fill -- and that is good, since filling a register for a known constant value is a waste.

It's not good to have dead code that covers unreachable conditions. I think it would be better to assert that our register is invalid and simply set edge to null.

[Mark Lam]

(In reply to comment #5)
> Comment on attachment 265511 [details]
> the patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265511&action=review
> 
> > Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:-502
> > -    ASSERT(isType(SpecHeapTop));
> 
> Can't you still assert byte code top?

Fixed.

> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2845
> > +        // Let's swap the operands.
> > +        ResultType tempType = rightType;
> > +        rightType = leftType;
> > +        leftType = tempType;
> 
> Instead of saying "let's swap" in a comment, you can call std::swap.

Thanks.  Learned something new today.

> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2877
> > +
> > +    if (leftIsConstInt32) {
> > +        // Since we may be dealing with non-numeric operands (which may not be commutative)
> > +        // in the slow path, we need to restore the left right order.
> > +        rightRegs = right.jsValueRegs();
> > +        leftRegs = resultRegs;
> > +        int64_t leftConst = node->child1()->asInt32();
> > +        m_jit.moveValue(JSValue(leftConst), leftRegs);
> 
> Can we avoid the swapping concern by simply building into JITAddGenerator
> the ability to swap left and right internally if left is constant?
> 
> In addition to reducing code at the call sites, that seems like a better
> separation of concerns. JITAddGenerator's client does not know that swapping
> is appropriate since it does not know whether all of JITAddGenerator's fast
> path operations are commutative. Only JITAddGenerator can know that.

Good point.  I will fix.

> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2587
> > +#if USE(JSVALUE64)
> > +        if (m_gprOrInvalid != InvalidGPRReg)
> > +            m_jit->unlock(m_gprOrInvalid);
> > +#elif USE(JSVALUE32_64)
> > +        if (m_isDouble && m_register.fpr != InvalidFPRReg)
> > +            m_jit->unlock(m_register.fpr);
> > +        else if (m_register.pair.tagGPR != InvalidGPRReg && m_register.pair.payloadGPR != InvalidGPRReg) {
> > +            m_jit->unlock(m_register.pair.tagGPR);
> > +            m_jit->unlock(m_register.pair.payloadGPR);
> > +        }
> > +#endif
> 
> Is there a condition where we fill a register and then set it to not needed?

Yes.  I have observed that on 32-bit x86, we do encounter a situation where a node is already filled an the time of JSValueOperand construction.  As a result, it auto fills the register and locks it even though we didn't tell it to.

> It looks like we only set not needed when we choose not to fill -- and that
> is good, since filling a register for a known constant value is a waste.

Yes, it is a waste.  One option is to add a mode to JSValueOperand's constructor to tell it not to autofill?  In practice, there's no additional cost with the fill at runtime execution.  It was already loaded by a previous node.  We are merely choosing not to use it.  But we do need to undo the lock.  Hence, the addition of setNotNeeded().  I can look into adding a mode to the constructor to prevent auto-fill.

> It's not good to have dead code that covers unreachable conditions. I think
> it would be better to assert that our register is invalid and simply set
> edge to null.

It is not unreachable (as described above) ... at least not as the code is today.

[Geoffrey Garen]

> > Is there a condition where we fill a register and then set it to not needed?
> 
> Yes.  I have observed that on 32-bit x86, we do encounter a situation where
> a node is already filled an the time of JSValueOperand construction.  As a
> result, it auto fills the register and locks it even though we didn't tell
> it to.

Got it.

I think the best way to represent this, rather than JSValueOperand::setNotNeeded(), is to use WTF::Optional<JSValueOperand> inside this function. That expresses that we may or may not need one, instead of saying "I need one" followed by "I didn't mean it".

Another way to model the same thing is to add to JSValueOperand a default constructor and a move constructor. But that seems more involved.

[Geoffrey Garen]

Comment on attachment 265511 [details]
the patch.

Clearing the review flag since it looks like we should make these changes.

[Geoffrey Garen]

Comment on attachment 265511 [details]
the patch.

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2887
> +
> +    if (isKnownNotNumber(node->child1().node()) || isKnownNotNumber(node->child2().node()))
> +        callOperation(operationValueAddNotNumber, resultRegs, leftRegs, rightRegs);
> +    else
> +        callOperation(operationValueAdd, resultRegs, leftRegs, rightRegs);

One more comment: This code is a bit out-of-sync with our generator. If we know that both children are not numbers, we'd really rather not call the generator at all. So, I guess we want that case up at the top, beside the constant case. Then the fact that we do ValueAdd on the slow path is always true.

[Mark Lam]

Created attachment 265690 [details]
patch 2: applied all of Geoff's feedback.

This patch has passed the JSC and layout tests.  Benchmarks are still being run.

[Mark Lam]

Created attachment 265691 [details]
x86 benchmark result.

[Mark Lam]

Created attachment 265693 [details]
x86_64 benchmark result.

[Mark Lam]

Comment on attachment 265690 [details]
patch 2: applied all of Geoff's feedback.

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2589
> +
> +    void setNotNeeded()
> +    {
> +#if USE(JSVALUE64)
> +        if (m_gprOrInvalid != InvalidGPRReg)
> +            m_jit->unlock(m_gprOrInvalid);
> +#elif USE(JSVALUE32_64)
> +        if (m_isDouble && m_register.fpr != InvalidFPRReg)
> +            m_jit->unlock(m_register.fpr);
> +        else if (m_register.pair.tagGPR != InvalidGPRReg && m_register.pair.payloadGPR != InvalidGPRReg) {
> +            m_jit->unlock(m_register.pair.tagGPR);
> +            m_jit->unlock(m_register.pair.payloadGPR);
> +        }
> +#endif
> +        m_edge = Edge();
> +    }

Eeek.  Stale changes.  This should be removed.

[Mark Lam]

Comment on attachment 265690 [details]
patch 2: applied all of Geoff's feedback.

Uploaded wrong patch.

[Mark Lam]

Created attachment 265694 [details]
patch 3: the real patch with Geoff's feedback applied.

[WebKit Commit Bot]

Attachment 265694 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:47:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:48:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:49:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITAddGenerator.h:50:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 4 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 265694 [details]
patch 3: the real patch with Geoff's feedback applied.

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

r=me

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2867
> +

Stray whitespace.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:2574
> +        other.m_edge = Edge();

Let's also reset other's 

        , m_gprOrInvalid(InvalidGPRReg)
        , m_isDouble(false)

[Mark Lam]

Thanks for the review.  Landed in r192531: <http://trac.webkit.org/r192531>.