The following tests are failing after r149152 on ARM thumb and traditional: fast/js/integer-division-neg2tothe32-by-neg1.html [ Failure ] fast/js/regress/negative-zero-modulo.html [ Failure ]
Created attachment 204435 [details] small js test
Created attachment 204436 [details] proposed fix
We shouldn't enable negative-zero check like "result == 0" back. Now the assembly code tries to check correct condition "dividend < 0 && result == 0", but does it wrong, because op1GPR register (where dividend is stored) is clobbered while calling operationFModOnInts() and fmod() functions. I have tried this patch, but the new temporary register is also clobbered: diff --git a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp --- a/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp +++ b/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp @@ -3047,9 +3047,11 @@ void SpeculativeJIT::compileSoftModulo(Node* node) // and then attempt to convert back. GPRReg op1GPR = op1.gpr(); GPRReg op2GPR = op2.gpr(); + GPRTemporary op1Save(this); FPRResult result(this); + m_jit.move(op1GPR, op1Save.gpr()); flushRegisters(); callOperation(operationFModOnInts, result.fpr(), op1GPR, op2GPR); @@ -3060,7 +3062,7 @@ void SpeculativeJIT::compileSoftModulo(Node* node) speculationCheck(Overflow, JSValueRegs(), 0, failureCases); if (!nodeCanIgnoreNegativeZero(node->arithNodeFlags())) { // Check that we're not about to create negative zero. - JITCompiler::Jump numeratorPositive = m_jit.branch32(JITCompiler::GreaterThanOrEqual, op1GPR, TrustedImm32(0)); + JITCompiler::Jump numeratorPositive = m_jit.branch32(JITCompiler::GreaterThanOrEqual, op1Save.gpr(), TrustedImm32(0)); speculationCheck(NegativeZero, JSValueRegs(), 0, m_jit.branchTest32(JITCompiler::Zero, intResult.gpr())); numeratorPositive.link(&m_jit); } I don't know what is a proper way to save dividend value for negative-zero checks.
*** Bug 105304 has been marked as a duplicate of this bug. ***
Adding the ARM meta bug, and please unskip them when you land the fix.
Created attachment 204579 [details] Proposed patch The problem may be fixed using silentSpillAllRegisters and silentFillAllRegisters around the call. Is it OK?
(In reply to comment #6) > Created an attachment (id=204579) [details] > Proposed patch > > The problem may be fixed using silentSpillAllRegisters and silentFillAllRegisters around the call. Is it OK? It worked for me and the generated code seems fine as well.
adding more JSC reviewer and ping for review
ping for review? Is there a JSC reviewer somewhere?
Is this patch up to date? I don't see a call to operationFModOnInts in trunk.
Comment on attachment 204579 [details] Proposed patch Assuming that patches for review since 2013 are stale, r-
These failures aren't exhibited anymore.