[ARMv6][DFG] ARM MacroAssembler is always emitting cmn when immediate is 0
authorticaiolima@gmail.com <ticaiolima@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 20 Jun 2017 00:50:23 +0000 (00:50 +0000)
committerticaiolima@gmail.com <ticaiolima@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 20 Jun 2017 00:50:23 +0000 (00:50 +0000)
https://bugs.webkit.org/show_bug.cgi?id=172972

Reviewed by Mark Lam.

We are changing internalCompare32 implementation in ARM
MacroAssembler to emit "cmp" when the "right.value" is 0.
It is generating wrong comparison cases, since the
semantics of cmn is opposite of cmp[1]. One case that it's breaking is
"branch32(MacroAssembler::Above, gpr, TrustedImm32(0))", where ends
resulting in following assembly code:

```
cmn $r0, #0
bhi <address>
```

However, as cmn is similar to "adds", it will never take the branch
when $r0 > 0. In that case, the correct opcode is "cmp". With this
patch we will fix current broken tests that uses
"branch32(MacroAssembler::Above, gpr, TrustedImm32(0))",
such as ForwardVarargs, Spread and GetRestLength.

[1] - http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0204j/Cihiddid.html

* assembler/MacroAssemblerARM.h:
(JSC::MacroAssemblerARM::internalCompare32):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@218519 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/assembler/MacroAssemblerARM.h

index f6f215c..ce1e57d 100644 (file)
@@ -1,3 +1,33 @@
+2017-06-19  Caio Lima  <ticaiolima@gmail.com>
+
+        [ARMv6][DFG] ARM MacroAssembler is always emitting cmn when immediate is 0
+        https://bugs.webkit.org/show_bug.cgi?id=172972
+
+        Reviewed by Mark Lam.
+
+        We are changing internalCompare32 implementation in ARM
+        MacroAssembler to emit "cmp" when the "right.value" is 0.
+        It is generating wrong comparison cases, since the
+        semantics of cmn is opposite of cmp[1]. One case that it's breaking is
+        "branch32(MacroAssembler::Above, gpr, TrustedImm32(0))", where ends
+        resulting in following assembly code:
+
+        ```
+        cmn $r0, #0
+        bhi <address>
+        ```
+
+        However, as cmn is similar to "adds", it will never take the branch
+        when $r0 > 0. In that case, the correct opcode is "cmp". With this
+        patch we will fix current broken tests that uses
+        "branch32(MacroAssembler::Above, gpr, TrustedImm32(0))",
+        such as ForwardVarargs, Spread and GetRestLength.
+
+        [1] - http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0204j/Cihiddid.html
+
+        * assembler/MacroAssemblerARM.h:
+        (JSC::MacroAssemblerARM::internalCompare32):
+
 2017-06-19  Joseph Pecoraro  <pecoraro@apple.com>
 
         test262: Completion values for control flow do not match the spec
index 991913b..bc15e49 100644 (file)
@@ -1600,7 +1600,7 @@ private:
 
     void internalCompare32(RegisterID left, TrustedImm32 right)
     {
-        ARMWord tmp = (static_cast<unsigned>(right.m_value) == 0x80000000) ? ARMAssembler::InvalidImmediate : m_assembler.getOp2(-right.m_value);
+        ARMWord tmp = (!right.value || static_cast<unsigned>(right.m_value) == 0x80000000) ? ARMAssembler::InvalidImmediate : m_assembler.getOp2(-right.m_value);
         if (tmp != ARMAssembler::InvalidImmediate)
             m_assembler.cmn(left, tmp);
         else
@@ -1609,7 +1609,7 @@ private:
 
     void internalCompare32(Address left, TrustedImm32 right)
     {
-        ARMWord tmp = (static_cast<unsigned>(right.m_value) == 0x80000000) ? ARMAssembler::InvalidImmediate : m_assembler.getOp2(-right.m_value);
+        ARMWord tmp = (!right.value || static_cast<unsigned>(right.m_value) == 0x80000000) ? ARMAssembler::InvalidImmediate : m_assembler.getOp2(-right.m_value);
         load32(left, ARMRegisters::S1);
         if (tmp != ARMAssembler::InvalidImmediate)
             m_assembler.cmn(ARMRegisters::S1, tmp);