Bug 117542 – ARM JSC negative zero check missing from compileSoftModulo() after r149152

• Status: RESOLVED
• Resolution: WONTFIX
• Component: JavaScriptCore

[Gabor Rapcsanyi]

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 ]

[Gabor Rapcsanyi]

Created attachment 204435 [details]
small js test

[Gabor Rapcsanyi]

Created attachment 204436 [details]
proposed fix

[Roman Zhuykov]

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.

[Csaba Osztrogonác]

*** Bug 105304 has been marked as a duplicate of this bug. ***

[Csaba Osztrogonác]

Adding the ARM meta bug, and please unskip them when you land the fix.

[Roman Zhuykov]

Created attachment 204579 [details]
Proposed patch

The problem may be fixed using silentSpillAllRegisters and silentFillAllRegisters around the call. Is it OK?

[Gabor Rapcsanyi]

(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.

[Csaba Osztrogonác]

adding more JSC reviewer and ping for review

[Csaba Osztrogonác]

ping for review? Is there a JSC reviewer somewhere?

[Geoffrey Garen]

Is this patch up to date? I don't see a call to operationFModOnInts in trunk.

[Brady Eidson]

Comment on attachment 204579 [details]
Proposed patch

Assuming that patches for review since 2013 are stale, r-

[Zan Dobersek]

These failures aren't exhibited anymore.