Changeset 196591 in webkit

Timestamp:

Feb 15, 2016, 1:08:47 PM (9 years ago)

Author:

mark.lam@apple.com

Message:

[ARMv7] stress/op_rshift.js and stress/op_urshift.js are failing.
​https://bugs.webkit.org/show_bug.cgi?id=151514

Reviewed by Filip Pizlo.

The issue turns out to be trivial: on ARMv7 (and traditional ARM too), arithmetic
shift right (ASR) and logical shift right (LSR) takes an immediate shift amount
from 1-32. See ​http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0204j/Cjacbgca.html.
An immediate shift amount of 0 is interpreted as a shift of 32 bits.

Meanwhile, our macro assembler is expecting the immediate shift value to be
between 0-31. As a result, a shift amount of 0 is being wrongly encoded with 0
bits which means shift right by 32 bits.

The fix is to check if the shift amount is 0, and if so, emit a move. Else,
emit the right shift as usual.

This issue does not affect left shifts, as the immediate shift amount for left
shifts is between 0-31 as our macro assembler expects.

• assembler/MacroAssemblerARM.h:

(JSC::MacroAssemblerARM::rshift32):
(JSC::MacroAssemblerARM::urshift32):
(JSC::MacroAssemblerARM::sub32):

• assembler/MacroAssemblerARMv7.h:

(JSC::MacroAssemblerARMv7::rshift32):
(JSC::MacroAssemblerARMv7::urshift32):

• tests/stress/op_rshift.js:
• tests/stress/op_urshift.js:
• Un-skip these tests. They should always pass now.

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• assembler/MacroAssemblerARM.h (modified) (2 diffs)
• assembler/MacroAssemblerARMv7.h (modified) (2 diffs)
• tests/stress/op_rshift.js (modified) (1 diff)
• tests/stress/op_urshift.js (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2016-02-15  Mark Lam  <mark.lam@apple.com>
+
+        [ARMv7] stress/op_rshift.js and stress/op_urshift.js are failing.
+        https://bugs.webkit.org/show_bug.cgi?id=151514
+
+        Reviewed by Filip Pizlo.
+
+        The issue turns out to be trivial: on ARMv7 (and traditional ARM too), arithmetic
+        shift right (ASR) and logical shift right (LSR) takes an immediate shift amount
+        from 1-32.  See http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0204j/Cjacbgca.html.
+        An immediate shift amount of 0 is interpreted as a shift of 32 bits.
+
+        Meanwhile, our macro assembler is expecting the immediate shift value to be
+        between 0-31.  As a result, a shift amount of 0 is being wrongly encoded with 0
+        bits which means shift right by 32 bits.
+
+        The fix is to check if the shift amount is 0, and if so, emit a move.  Else,
+        emit the right shift as usual.
+
+        This issue does not affect left shifts, as the immediate shift amount for left
+        shifts is between 0-31 as our macro assembler expects.
+
+        * assembler/MacroAssemblerARM.h:
+        (JSC::MacroAssemblerARM::rshift32):
+        (JSC::MacroAssemblerARM::urshift32):
+        (JSC::MacroAssemblerARM::sub32):
+        * assembler/MacroAssemblerARMv7.h:
+        (JSC::MacroAssemblerARMv7::rshift32):
+        (JSC::MacroAssemblerARMv7::urshift32):
+
+        * tests/stress/op_rshift.js:
+        * tests/stress/op_urshift.js:
+        - Un-skip these tests.  They should always pass now.
+
 2016-02-15  Filip Pizlo  <fpizlo@apple.com>
 

trunk/Source/JavaScriptCore/assembler/MacroAssemblerARM.h

@@ -275 +275 @@
     void rshift32(RegisterID src, TrustedImm32 imm, RegisterID dest)
     {
-        m_assembler.movs(dest, m_assembler.asr(src, imm.m_value & 0x1f));
+        if (!imm.m_value)
+            move(src, dest);
+        else
+            m_assembler.movs(dest, m_assembler.asr(src, imm.m_value & 0x1f));
     }
 
@@ -298 +301 @@
     void urshift32(RegisterID src, TrustedImm32 imm, RegisterID dest)
     {
-        m_assembler.movs(dest, m_assembler.lsr(src, imm.m_value & 0x1f));
+        if (!imm.m_value)
+            move(src, dest);
+        else
+            m_assembler.movs(dest, m_assembler.lsr(src, imm.m_value & 0x1f));
     }
 

trunk/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h

@@ -401 +401 @@
     void rshift32(RegisterID src, TrustedImm32 imm, RegisterID dest)
     {
-        m_assembler.asr(dest, src, imm.m_value & 0x1f);
+        if (!imm.m_value)
+            move(src, dest);
+        else
+            m_assembler.asr(dest, src, imm.m_value & 0x1f);
     }
 
@@ -426 +429 @@
     void urshift32(RegisterID src, TrustedImm32 imm, RegisterID dest)
     {
-        m_assembler.lsr(dest, src, imm.m_value & 0x1f);
+        if (!imm.m_value)
+            move(src, dest);
+        else
+            m_assembler.lsr(dest, src, imm.m_value & 0x1f);
     }
 

trunk/Source/JavaScriptCore/tests/stress/op_rshift.js

@@ -1 @@
-//@ skip
-// FIXME: https://bugs.webkit.org/show_bug.cgi?id=151514
+//@ runFTLNoCJIT
 
 // If all goes well, this test module will terminate silently. If not, it will print

trunk/Source/JavaScriptCore/tests/stress/op_urshift.js

@@ -1 @@
-//@ skip
-// FIXME: https://bugs.webkit.org/show_bug.cgi?id=151514
+//@ runFTLNoCJIT
 
 // If all goes well, this test module will terminate silently. If not, it will print