Skip to content

Commit

Permalink
[JSC] Fix ArithMin/ArithMax handling for double, and Int32 speculation
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=246422
rdar://101048572

Reviewed by Justin Michaud.

This patch fixes two issues: one is just a bug fix and one is a bit theoretical.

1. FTL ArithMin/ArithMax double case is skipping the first parameter accidentally. This patch fixes it.
2. DFG ArithMin/ArithMax Int32 case can OSR exit after clobbering first parameter's register, which can theoretically
   puts different output if type speculation failed in the second or later parameters.

* JSTests/stress/arith-min-max-multiple.js:
(test7):
(test8):
* Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:
* Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileArithMinOrMax):

Canonical link: https://commits.webkit.org/255465@main
  • Loading branch information
Constellation committed Oct 13, 2022
1 parent f07375f commit 6e034bb
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 2 deletions.
14 changes: 14 additions & 0 deletions JSTests/stress/arith-min-max-multiple.js
Expand Up @@ -39,6 +39,18 @@ function test6(num)
}
noInline(test6);

function test7(num)
{
return Math.max(44.3, 0.2, 0.3, -1.3, 2.5, num);
}
noInline(test7);

function test8(num)
{
return Math.min(-44.3, 0.2, 0.3, -1.3, 2.5, num);
}
noInline(test8);

for (let i = 0; i < 1e5; ++i) {
shouldBe(test1(0), -4);
shouldBe(test1(-100), -100);
Expand All @@ -50,4 +62,6 @@ for (let i = 0; i < 1e5; ++i) {
shouldBe(test4(1e1000), 1e1000);
shouldBe(Number.isNaN(test5(0)), true);
shouldBe(Number.isNaN(test6(2000.1)), true);
shouldBe(test7(10), 44.3);
shouldBe(test8(10), -44.3);
}
26 changes: 25 additions & 1 deletion Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Expand Up @@ -7146,8 +7146,32 @@ void SpeculativeJIT::compileArithMinMax(Node* node)
{
switch (m_graph.child(node, 0).useKind()) {
case Int32Use: {
if (node->numChildren() == 2) {
// Optimize this pattern since it is the most common one.
SpeculateStrictInt32Operand op1(this, m_graph.child(node, 0));
SpeculateStrictInt32Operand op2(this, m_graph.child(node, 1));
GPRTemporary result(this, Reuse, op1);

GPRReg op1GPR = op1.gpr();
GPRReg op2GPR = op2.gpr();
GPRReg resultGPR = result.gpr();

m_jit.move(op1GPR, resultGPR);

#if CPU(ARM64) || CPU(X86_64)
m_jit.moveConditionally32(node->op() == ArithMin ? MacroAssembler::GreaterThan : MacroAssembler::LessThan, resultGPR, op2GPR, op2GPR, resultGPR);
#else
auto resultLess = m_jit.branch32(node->op() == ArithMin ? MacroAssembler::LessThan : MacroAssembler::GreaterThan, resultGPR, op2GPR);
m_jit.move(op2GPR, resultGPR);
resultLess.link(&m_jit);
#endif

strictInt32Result(resultGPR, node);
break;
}

SpeculateStrictInt32Operand op1(this, m_graph.child(node, 0));
GPRTemporary result(this, Reuse, op1);
GPRTemporary result(this); // Do not use Reuse because Int32Use speculation can fail in the following loop.

GPRReg op1GPR = op1.gpr();
GPRReg resultGPR = result.gpr();
Expand Down
2 changes: 1 addition & 1 deletion Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
Expand Up @@ -3010,7 +3010,7 @@ class LowerDFGToB3 {
ScratchBuffer* scratchBuffer = vm().scratchBufferForSize(scratchSize);
LValue buffer = m_out.constIntPtr(static_cast<const double*>(scratchBuffer->dataBuffer()));

for (unsigned index = 1; index < m_node->numChildren(); ++index) {
for (unsigned index = 0; index < m_node->numChildren(); ++index) {
LValue value = lowDouble(m_graph.child(m_node, index));
m_out.storeDouble(value, m_out.baseIndex(m_heaps.indexedDoubleProperties, buffer, m_out.constInt32(index), jsNumber(index)));
}
Expand Down

0 comments on commit 6e034bb

Please sign in to comment.