Bug 117542

Summary: ARM JSC negative zero check missing from compileSoftModulo() after r149152
Product: WebKit Reporter: Gabor Rapcsanyi <rgabor>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: barraclough, fpizlo, ggaren, msaboff, oliver, ossy, zan, zherczeg, zhroma
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Linux   
Bug Depends on:    
Bug Blocks: 108645    
Attachments:
Description Flags
small js test
none
proposed fix
none
Proposed patch beidson: review-

Gabor Rapcsanyi
Reported 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 ]
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
DetailsFormatted DiffDiff
Proposed patch (5.04 KB, patch)
2013-06-13 06:22 PDT, Roman Zhuykov 		beidson: review-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Gabor Rapcsanyi
Comment 1 2013-06-12 07:23:56 PDT
Created attachment 204435 [details] small js test
Gabor Rapcsanyi
Comment 2 2013-06-12 07:24:39 PDT
Created attachment 204436 [details] proposed fix
Roman Zhuykov
Comment 3 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.
Csaba Osztrogonác
Comment 4 2013-06-13 03:09:15 PDT
*** Bug 105304 has been marked as a duplicate of this bug. ***
Csaba Osztrogonác
Comment 5 2013-06-13 03:09:55 PDT
Adding the ARM meta bug, and please unskip them when you land the fix.
Roman Zhuykov
Comment 6 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?
Gabor Rapcsanyi
Comment 7 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.
Csaba Osztrogonác
Comment 8 2013-07-02 02:16:08 PDT
adding more JSC reviewer and ping for review
Csaba Osztrogonác
Comment 9 2013-08-27 06:34:28 PDT
ping for review? Is there a JSC reviewer somewhere?
Geoffrey Garen
Comment 10 2013-08-27 11:13:07 PDT
Is this patch up to date? I don't see a call to operationFModOnInts in trunk.
Brady Eidson
Comment 11 2016-05-24 22:00:34 PDT
Comment on attachment 204579 [details] Proposed patch Assuming that patches for review since 2013 are stale, r-
Zan Dobersek
Comment 12 2017-10-18 01:53:00 PDT
These failures aren't exhibited anymore.
Note You need to log in before you can comment on or make changes to this bug.