Bug 226755 – Optimize compareStrictEq when neither side is a double and at least one is not a BigInt

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

[Robin Morisset]

Fairly similar to https://bugs.webkit.org/show_bug.cgi?id=226676, but catches more cases at the cost of more code being generated (since it now must deal with the string equality case).

[Robin Morisset]

Created attachment 430805 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/79321542>

[Robin Morisset]

Created attachment 431593 [details]
Patch

Fix 2 bugs in the speculativeJIT:
- doing register allocation within a branch
- doing an update to the interpreter state within a branch.

[Robin Morisset]

Created attachment 435639 [details]
Patch

[Yusuke Suzuki]

Debug tests are crashing (need to add appropriate doesGC handling).

[Robin Morisset]

Created attachment 435649 [details]
Patch

Sorry for that, I should have waited on EWS before sending this patch for review.

[Yusuke Suzuki]

Comment on attachment 435649 [details]
Patch

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

r=me with comments.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7470
> +void SpeculativeJIT::emitBitwiseJSValueEquality(JSValueOperand& left, JSValueOperand& right, GPRTemporary& result)

Let's just take `JSValueRegs left` and `JSValueRegs right` instead of `JSValueOperand&`. And let's just take GPRReg for result.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7482
> +void SpeculativeJIT::emitBranchOnBitwiseJSValueEquality(JSValueOperand& left, JSValueOperand& right, BasicBlock* taken, BasicBlock* notTaken)

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7523
> +    emitBitwiseJSValueEquality(left, right, result);

Let's just pass leftRegs, rightRegs, and resultGPR.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7543
> +    emitBranchOnBitwiseJSValueEquality(left, right, taken, notTaken);

Lef's just pass leftRegs and rightRegs.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:356
> +    GPRReg leftGPR = leftRegs.payloadGPR();
> +    GPRReg rightGPR = rightRegs.payloadGPR();

I think using `leftRegs.payloadGPR()` / `rightRegs.payloadGPR()` is easier to read since we do not need to remember leftGPR is a part of leftRegs.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9749
> +        m_out.branch(isCell(leftValue, provenType(neitherDoubleNorHeapBigIntEdge)), unsure(leftIsCellEqualCase), unsure(returnTrueBlock));

We can remove Numbers from provenType. (provenType(neitherDoubleNorHeapBigIntEdge) & ~SpecFullNumber)

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9763
> +        m_out.branch(isNotCell(leftValue, leftValueType), unsure(continuation), unsure(leftIsCell));

We can remove doubles from the provenType.  (provenType(neitherDoubleNorHeapBigIntEdge) & ~SpecFullDouble)

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9767
> +        m_out.branch(isNotString(leftValue, leftValueType), unsure(continuation), unsure(leftIsString));

We can remove non-cell and HeapBigInt from provenType.  (provenType(neitherDoubleNorHeapBigIntEdge) & SpecCell & ~SpecHeapBigInt).

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9770
> +        m_out.branch(isNotCell(rightValue, rightValueType), unsure(continuation), unsure(rightIsCell));

We can remove doubles from the provenType. (provenType(notDoubleEdge) & ~SpecFullDouble).

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9773
> +        m_out.branch(isNotString(rightValue, rightValueType), unsure(continuation), unsure(rightIsString));

We can remove non-cells from the provenType. (provenType(notDoubleEdge) & SpecCell).

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:18876
> +        m_out.branch(isCell(value, provenType(edge)), unsure(isCellBlock), unsure(continuation));

We can remove Int32 (but we can also remove Numbers) from the provenType.  (provenType(edge) & ~SpecFullNumber)

[Robin Morisset]

Thanks for the review and the suggestions. I've answered them inline below.

(In reply to Yusuke Suzuki from comment #7)
> Comment on attachment 435649 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435649&action=review
> 
> r=me with comments.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7470
> > +void SpeculativeJIT::emitBitwiseJSValueEquality(JSValueOperand& left, JSValueOperand& right, GPRTemporary& result)
> 
> Let's just take `JSValueRegs left` and `JSValueRegs right` instead of
> `JSValueOperand&`. And let's just take GPRReg for result.

done.
 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7482
> > +void SpeculativeJIT::emitBranchOnBitwiseJSValueEquality(JSValueOperand& left, JSValueOperand& right, BasicBlock* taken, BasicBlock* notTaken)
> 
> Ditto.

done.
 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7523
> > +    emitBitwiseJSValueEquality(left, right, result);
> 
> Let's just pass leftRegs, rightRegs, and resultGPR.

done.

> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:7543
> > +    emitBranchOnBitwiseJSValueEquality(left, right, taken, notTaken);
> 
> Lef's just pass leftRegs and rightRegs.

done.
 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:356
> > +    GPRReg leftGPR = leftRegs.payloadGPR();
> > +    GPRReg rightGPR = rightRegs.payloadGPR();
> 
> I think using `leftRegs.payloadGPR()` / `rightRegs.payloadGPR()` is easier
> to read since we do not need to remember leftGPR is a part of leftRegs.

I am not convinced: leftGPR corresponding to leftRegs and rightGPR corresponding to rightRegs is fairly easy to remember, and we would have to repeat the ".payloadGPR()" everywhere.

> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9749
> > +        m_out.branch(isCell(leftValue, provenType(neitherDoubleNorHeapBigIntEdge)), unsure(leftIsCellEqualCase), unsure(returnTrueBlock));
> 
> We can remove Numbers from provenType.
> (provenType(neitherDoubleNorHeapBigIntEdge) & ~SpecFullNumber)

I've removed Int32 from the type (since it is what we just checked for). I don't see how we can be confident that Double won't be there.

> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9763
> > +        m_out.branch(isNotCell(leftValue, leftValueType), unsure(continuation), unsure(leftIsCell));
> 
> We can remove doubles from the provenType. 
> (provenType(neitherDoubleNorHeapBigIntEdge) & ~SpecFullDouble)

done.

> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9767
> > +        m_out.branch(isNotString(leftValue, leftValueType), unsure(continuation), unsure(leftIsString));
> 
> We can remove non-cell and HeapBigInt from provenType. 
> (provenType(neitherDoubleNorHeapBigIntEdge) & SpecCell & ~SpecHeapBigInt).

done.

> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9770
> > +        m_out.branch(isNotCell(rightValue, rightValueType), unsure(continuation), unsure(rightIsCell));
> 
> We can remove doubles from the provenType. (provenType(notDoubleEdge) &
> ~SpecFullDouble).

done.

> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9773
> > +        m_out.branch(isNotString(rightValue, rightValueType), unsure(continuation), unsure(rightIsString));
> 
> We can remove non-cells from the provenType. (provenType(notDoubleEdge) &
> SpecCell).

done. I have also removed doubles.

> > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:18876
> > +        m_out.branch(isCell(value, provenType(edge)), unsure(isCellBlock), unsure(continuation));
> 
> We can remove Int32 (but we can also remove Numbers) from the provenType. 
> (provenType(edge) & ~SpecFullNumber)

done.

[Robin Morisset]

Created attachment 437677 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 435649 [details]
Patch

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

>>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9749
>>> +        m_out.branch(isCell(leftValue, provenType(neitherDoubleNorHeapBigIntEdge)), unsure(leftIsCellEqualCase), unsure(returnTrueBlock));
>> 
>> We can remove Numbers from provenType. (provenType(neitherDoubleNorHeapBigIntEdge) & ~SpecFullNumber)
> 
> I've removed Int32 from the type (since it is what we just checked for). I don't see how we can be confident that Double won't be there.

Because we already performed the typecheck onto the leftValue above, we will not get numbers here.

[Robin Morisset]

(In reply to Yusuke Suzuki from comment #10)
> Comment on attachment 435649 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435649&action=review
> 
> >>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9749
> >>> +        m_out.branch(isCell(leftValue, provenType(neitherDoubleNorHeapBigIntEdge)), unsure(leftIsCellEqualCase), unsure(returnTrueBlock));
> >> 
> >> We can remove Numbers from provenType. (provenType(neitherDoubleNorHeapBigIntEdge) & ~SpecFullNumber)
> > 
> > I've removed Int32 from the type (since it is what we just checked for). I don't see how we can be confident that Double won't be there.
> 
> Because we already performed the typecheck onto the leftValue above, we will
> not get numbers here.

You're right, not sure how I missed it. Fixing it.

[Robin Morisset]

Created attachment 437695 [details]
Patch

[EWS]

ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!.

[Yusuke Suzuki]

Created attachment 437716 [details]
Patch

Patch for landing

[EWS]

Committed r282200 (241487@main): <https://commits.webkit.org/241487@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437716 [details].