Bug 117542 - ARM JSC negative zero check missing from compileSoftModulo() after r149152
Summary: ARM JSC negative zero check missing from compileSoftModulo() after r149152
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 105304 (view as bug list)
Depends on:
Blocks: 108645
  Show dependency treegraph
 
Reported: 2013-06-12 07:22 PDT by Gabor Rapcsanyi
Modified: 2017-10-18 01:53 PDT (History)
9 users (show)

See Also:


Attachments
small js test (224 bytes, text/plain)
2013-06-12 07:23 PDT, Gabor Rapcsanyi
no flags Details
proposed fix (1.52 KB, patch)
2013-06-12 07:24 PDT, Gabor Rapcsanyi
no flags Details | Formatted Diff | Diff
Proposed patch (5.04 KB, patch)
2013-06-13 06:22 PDT, Roman Zhuykov
beidson: review-
zhroma: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gabor Rapcsanyi 2013-06-12 07:22:14 PDT
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 ]
Comment 1 Gabor Rapcsanyi 2013-06-12 07:23:56 PDT
Created attachment 204435 [details]
small js test
Comment 2 Gabor Rapcsanyi 2013-06-12 07:24:39 PDT
Created attachment 204436 [details]
proposed fix
Comment 3 Roman Zhuykov 2013-06-12 10:11:40 PDT
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.
Comment 4 Csaba Osztrogonác 2013-06-13 03:09:15 PDT
*** Bug 105304 has been marked as a duplicate of this bug. ***
Comment 5 Csaba Osztrogonác 2013-06-13 03:09:55 PDT
Adding the ARM meta bug, and please unskip them when you land the fix.
Comment 6 Roman Zhuykov 2013-06-13 06:22:52 PDT
Created attachment 204579 [details]
Proposed patch

The problem may be fixed using silentSpillAllRegisters and silentFillAllRegisters around the call. Is it OK?
Comment 7 Gabor Rapcsanyi 2013-06-13 06:51:05 PDT
(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.
Comment 8 Csaba Osztrogonác 2013-07-02 02:16:08 PDT
adding more JSC reviewer and ping for review
Comment 9 Csaba Osztrogonác 2013-08-27 06:34:28 PDT
ping for review? Is there a JSC reviewer somewhere?
Comment 10 Geoffrey Garen 2013-08-27 11:13:07 PDT
Is this patch up to date? I don't see a call to operationFModOnInts in trunk.
Comment 11 Brady Eidson 2016-05-24 22:00:34 PDT
Comment on attachment 204579 [details]
Proposed patch

Assuming that patches for review since 2013 are stale, r-
Comment 12 Zan Dobersek 2017-10-18 01:53:00 PDT
These failures aren't exhibited anymore.