Changeset 260802 in webkit

Timestamp:

Apr 27, 2020 5:35:45 PM (4 years ago)

Author:

sbarati@apple.com

Message:

compilePeepHoleBigInt32Branch needs to handle all conditions
​https://bugs.webkit.org/show_bug.cgi?id=211096
<rdar://problem/62469971>

Reviewed by Yusuke Suzuki.

We were falling through to the generic path for all conditions which
weren't Equal/NotEqual. The generic path does not do speculation, so
it was leading to potential miscompiles because we omitted a type check.
Defining compilePeepHoleBigInt32Branch for other conditions is trivial,
so this patch just implements that.

This failure is caught by microbenchmarks/sunspider-sha1-big-int.js

• dfg/DFGSpeculativeJIT.cpp:

(JSC::DFG::SpeculativeJIT::compilePeepHoleBranch):

• dfg/DFGSpeculativeJIT64.cpp:

(JSC::DFG::SpeculativeJIT::compilePeepHoleBigInt32Branch):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGSpeculativeJIT.cpp (modified) (1 diff)
• dfg/DFGSpeculativeJIT64.cpp (modified) (2 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2020-04-27  Saam Barati  <sbarati@apple.com>
+
+        compilePeepHoleBigInt32Branch needs to handle all conditions
+        https://bugs.webkit.org/show_bug.cgi?id=211096
+        <rdar://problem/62469971>
+
+        Reviewed by Yusuke Suzuki.
+
+        We were falling through to the generic path for all conditions which
+        weren't Equal/NotEqual. The generic path does not do speculation, so
+        it was leading to potential miscompiles because we omitted a type check.
+        Defining compilePeepHoleBigInt32Branch for other conditions is trivial,
+        so this patch just implements that.
+
+        This failure is caught by microbenchmarks/sunspider-sha1-big-int.js
+
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compilePeepHoleBranch):
+        * dfg/DFGSpeculativeJIT64.cpp:
+        (JSC::DFG::SpeculativeJIT::compilePeepHoleBigInt32Branch):
+
 2020-04-27  Jason Lawrence  <lawrence.j@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp

@@ -1717 +1717 @@
             compilePeepHoleInt32Branch(node, branchNode, condition);
 #if USE(BIGINT32)
-        else if (node->isBinaryUseKind(BigInt32Use) && (condition == MacroAssembler::Equal || condition == MacroAssembler::NotEqual))
+        else if (node->isBinaryUseKind(BigInt32Use))
             compilePeepHoleBigInt32Branch(node, branchNode, condition);
 #endif

trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp

@@ -1768 +1768 @@
 void SpeculativeJIT::compilePeepHoleBigInt32Branch(Node* node, Node* branchNode, JITCompiler::RelationalCondition condition)
 {
-    // Other conditions would require unboxing the BigInt32 to get correct results on negative numbers.
-    ASSERT(condition == MacroAssembler::Equal || condition == MacroAssembler::NotEqual);
-
     BasicBlock* taken = branchNode->branchData()->taken.block;
     BasicBlock* notTaken = branchNode->branchData()->notTaken.block;
@@ -1778 +1775 @@
     if (taken == nextBlock()) {
         condition = JITCompiler::invert(condition);
-        BasicBlock* tmp = taken;
-        taken = notTaken;
-        notTaken = tmp;
+        std::swap(taken, notTaken);
     }
 
     SpeculateBigInt32Operand op1(this, node->child1());
     SpeculateBigInt32Operand op2(this, node->child2());
-
-    branch64(condition, op1.gpr(), op2.gpr(), taken);
-    jump(notTaken);
+    GPRReg op1GPR = op1.gpr();
+    GPRReg op2GPR = op2.gpr();
+
+    if (condition == MacroAssembler::Equal || condition == MacroAssembler::NotEqual) {
+        branch64(condition, op1GPR, op2GPR, taken);
+        jump(notTaken);
+    } else {
+        GPRTemporary lhs(this, Reuse, op1);
+        GPRTemporary rhs(this, Reuse, op2);
+        GPRReg lhsGPR = lhs.gpr();
+        GPRReg rhsGPR = rhs.gpr();
+        m_jit.unboxBigInt32(op1GPR, lhsGPR);
+        m_jit.unboxBigInt32(op2GPR, rhsGPR);
+        branch32(condition, lhsGPR, rhsGPR, taken);
+        jump(notTaken);
+    }
+
 }
 #endif // USE(BIGINT32)