[JSC] ZeroExtend and SignExtend use incorrect addressing on ARM64
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 13 Feb 2016 04:48:40 +0000 (04:48 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 13 Feb 2016 04:48:40 +0000 (04:48 +0000)
https://bugs.webkit.org/show_bug.cgi?id=154208

Patch by Benjamin Poulain <bpoulain@apple.com> on 2016-02-12
Reviewed by Filip Pizlo.

When lowering:
    @1 = Load32(@x)
    @2 = SExt8(@1)

LowerToAir would see there is a form of SignExtend8To32 (an alias for Load8S)
and use that.

There are two problems with that:
1) If we have an Addr, it went through legalizeMemoryOffsets() for a 32bits
   load. If used on an other kind of load, there is no guarantee the addressing
   is still valid.
2) If we have an Index, it is computed for the 32bits MemoryValue.
   The computed index is not valid for the 8bits load.

(2) could be fixed by changing LowerToAir to use the current instruction width
instead of the B3ValueWidth but that's a bit tricky. We should just embrace
that one of our target is a Load-Store architecture.

In this patch, I just disabled the faulty forms on ARM64. We still need those operations
to be fast, this will be addressed in: https://bugs.webkit.org/show_bug.cgi?id=154207

I also strengthened the m_allowScratchRegister assertion. The instructions that do not
invalidate the temporary did not run the assertion, making this harder to debug.

* assembler/MacroAssemblerARM64.h:
(JSC::MacroAssemblerARM64::load8):
(JSC::MacroAssemblerARM64::store64):
(JSC::MacroAssemblerARM64::store32):
(JSC::MacroAssemblerARM64::loadDouble):
(JSC::MacroAssemblerARM64::storeDouble):
(JSC::MacroAssemblerARM64::branch32):
(JSC::MacroAssemblerARM64::branch64):
(JSC::MacroAssemblerARM64::getCachedDataTempRegisterIDAndInvalidate):
(JSC::MacroAssemblerARM64::getCachedMemoryTempRegisterIDAndInvalidate):
(JSC::MacroAssemblerARM64::dataMemoryTempRegister):
(JSC::MacroAssemblerARM64::cachedMemoryTempRegister):
(JSC::MacroAssemblerARM64::load):
(JSC::MacroAssemblerARM64::store):
* b3/air/AirOpcode.opcodes:

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/assembler/MacroAssemblerARM64.h
Source/JavaScriptCore/b3/air/AirOpcode.opcodes

index 701b27b..54a1a2e 100644 (file)
@@ -1,3 +1,50 @@
+2016-02-12  Benjamin Poulain  <bpoulain@apple.com>
+
+        [JSC] ZeroExtend and SignExtend use incorrect addressing on ARM64
+        https://bugs.webkit.org/show_bug.cgi?id=154208
+
+        Reviewed by Filip Pizlo.
+
+        When lowering:
+            @1 = Load32(@x)
+            @2 = SExt8(@1)
+
+        LowerToAir would see there is a form of SignExtend8To32 (an alias for Load8S)
+        and use that.
+
+        There are two problems with that:
+        1) If we have an Addr, it went through legalizeMemoryOffsets() for a 32bits
+           load. If used on an other kind of load, there is no guarantee the addressing
+           is still valid.
+        2) If we have an Index, it is computed for the 32bits MemoryValue.
+           The computed index is not valid for the 8bits load.
+
+        (2) could be fixed by changing LowerToAir to use the current instruction width
+        instead of the B3ValueWidth but that's a bit tricky. We should just embrace
+        that one of our target is a Load-Store architecture.
+
+        In this patch, I just disabled the faulty forms on ARM64. We still need those operations
+        to be fast, this will be addressed in: https://bugs.webkit.org/show_bug.cgi?id=154207
+
+        I also strengthened the m_allowScratchRegister assertion. The instructions that do not
+        invalidate the temporary did not run the assertion, making this harder to debug.
+
+        * assembler/MacroAssemblerARM64.h:
+        (JSC::MacroAssemblerARM64::load8):
+        (JSC::MacroAssemblerARM64::store64):
+        (JSC::MacroAssemblerARM64::store32):
+        (JSC::MacroAssemblerARM64::loadDouble):
+        (JSC::MacroAssemblerARM64::storeDouble):
+        (JSC::MacroAssemblerARM64::branch32):
+        (JSC::MacroAssemblerARM64::branch64):
+        (JSC::MacroAssemblerARM64::getCachedDataTempRegisterIDAndInvalidate):
+        (JSC::MacroAssemblerARM64::getCachedMemoryTempRegisterIDAndInvalidate):
+        (JSC::MacroAssemblerARM64::dataMemoryTempRegister):
+        (JSC::MacroAssemblerARM64::cachedMemoryTempRegister):
+        (JSC::MacroAssemblerARM64::load):
+        (JSC::MacroAssemblerARM64::store):
+        * b3/air/AirOpcode.opcodes:
+
 2016-02-12  Michael Saboff  <msaboff@apple.com>
 
         offlineasm: Emit Dwarf2 file and location directives to allow for debugging .asm files
index 637e5ae..8063237 100644 (file)
@@ -1049,10 +1049,10 @@ public:
     
     void load8(const void* address, RegisterID dest)
     {
-        moveToCachedReg(TrustedImmPtr(address), m_cachedMemoryTempRegister);
+        moveToCachedReg(TrustedImmPtr(address), cachedMemoryTempRegister());
         m_assembler.ldrb(dest, memoryTempRegister, ARM64Registers::zr);
         if (dest == memoryTempRegister)
-            m_cachedMemoryTempRegister.invalidate();
+            cachedMemoryTempRegister().invalidate();
     }
 
     void load8SignedExtendTo32(ImplicitAddress address, RegisterID dest)
@@ -1124,7 +1124,7 @@ public:
             return;
         }
 
-        moveToCachedReg(imm, m_dataMemoryTempRegister);
+        moveToCachedReg(imm, dataMemoryTempRegister());
         store64(dataTempRegister, address);
     }
 
@@ -1135,7 +1135,7 @@ public:
             return;
         }
 
-        moveToCachedReg(imm, m_dataMemoryTempRegister);
+        moveToCachedReg(imm, dataMemoryTempRegister());
         store64(dataTempRegister, address);
     }
     
@@ -1180,7 +1180,7 @@ public:
             return;
         }
 
-        moveToCachedReg(imm, m_dataMemoryTempRegister);
+        moveToCachedReg(imm, dataMemoryTempRegister());
         store32(dataTempRegister, address);
     }
 
@@ -1191,7 +1191,7 @@ public:
             return;
         }
 
-        moveToCachedReg(imm, m_dataMemoryTempRegister);
+        moveToCachedReg(imm, dataMemoryTempRegister());
         store32(dataTempRegister, address);
     }
 
@@ -1202,7 +1202,7 @@ public:
             return;
         }
 
-        moveToCachedReg(imm, m_dataMemoryTempRegister);
+        moveToCachedReg(imm, dataMemoryTempRegister());
         store32(dataTempRegister, address);
     }
 
@@ -1497,7 +1497,7 @@ public:
     
     void loadDouble(TrustedImmPtr address, FPRegisterID dest)
     {
-        moveToCachedReg(address, m_cachedMemoryTempRegister);
+        moveToCachedReg(address, cachedMemoryTempRegister());
         m_assembler.ldr<64>(dest, memoryTempRegister, ARM64Registers::zr);
     }
 
@@ -1642,7 +1642,7 @@ public:
 
     void storeDouble(FPRegisterID src, TrustedImmPtr address)
     {
-        moveToCachedReg(address, m_cachedMemoryTempRegister);
+        moveToCachedReg(address, cachedMemoryTempRegister());
         m_assembler.str<64>(src, memoryTempRegister, ARM64Registers::zr);
     }
 
@@ -1900,7 +1900,7 @@ public:
         else if (isUInt12(-right.m_value))
             m_assembler.cmn<32>(left, UInt12(-right.m_value));
         else {
-            moveToCachedReg(right, m_dataMemoryTempRegister);
+            moveToCachedReg(right, dataMemoryTempRegister());
             m_assembler.cmp<32>(left, dataTempRegister);
         }
         return Jump(makeBranch(cond));
@@ -1966,7 +1966,7 @@ public:
         else if (isUInt12(-immediate))
             m_assembler.cmn<64>(left, UInt12(static_cast<int32_t>(-immediate)));
         else {
-            moveToCachedReg(right, m_dataMemoryTempRegister);
+            moveToCachedReg(right, dataMemoryTempRegister());
             m_assembler.cmp<64>(left, dataTempRegister);
         }
         return Jump(makeBranch(cond));
@@ -2877,12 +2877,22 @@ private:
     ALWAYS_INLINE RegisterID getCachedDataTempRegisterIDAndInvalidate()
     {
         RELEASE_ASSERT(m_allowScratchRegister);
-        return m_dataMemoryTempRegister.registerIDInvalidate();
+        return dataMemoryTempRegister().registerIDInvalidate();
     }
     ALWAYS_INLINE RegisterID getCachedMemoryTempRegisterIDAndInvalidate()
     {
         RELEASE_ASSERT(m_allowScratchRegister);
-        return m_cachedMemoryTempRegister.registerIDInvalidate();
+        return cachedMemoryTempRegister().registerIDInvalidate();
+    }
+    ALWAYS_INLINE CachedTempRegister& dataMemoryTempRegister()
+    {
+        RELEASE_ASSERT(m_allowScratchRegister);
+        return m_dataMemoryTempRegister;
+    }
+    ALWAYS_INLINE CachedTempRegister& cachedMemoryTempRegister()
+    {
+        RELEASE_ASSERT(m_allowScratchRegister);
+        return m_cachedMemoryTempRegister;
     }
 
     ALWAYS_INLINE bool isInIntRange(intptr_t value)
@@ -3015,12 +3025,12 @@ private:
     ALWAYS_INLINE void load(const void* address, RegisterID dest)
     {
         intptr_t currentRegisterContents;
-        if (m_cachedMemoryTempRegister.value(currentRegisterContents)) {
+        if (cachedMemoryTempRegister().value(currentRegisterContents)) {
             intptr_t addressAsInt = reinterpret_cast<intptr_t>(address);
             intptr_t addressDelta = addressAsInt - currentRegisterContents;
 
             if (dest == memoryTempRegister)
-                m_cachedMemoryTempRegister.invalidate();
+                cachedMemoryTempRegister().invalidate();
 
             if (isInIntRange(addressDelta)) {
                 if (ARM64Assembler::canEncodeSImmOffset(addressDelta)) {
@@ -3036,7 +3046,7 @@ private:
 
             if ((addressAsInt & (~maskHalfWord0)) == (currentRegisterContents & (~maskHalfWord0))) {
                 m_assembler.movk<64>(memoryTempRegister, addressAsInt & maskHalfWord0, 0);
-                m_cachedMemoryTempRegister.setValue(reinterpret_cast<intptr_t>(address));
+                cachedMemoryTempRegister().setValue(reinterpret_cast<intptr_t>(address));
                 m_assembler.ldr<datasize>(dest, memoryTempRegister, ARM64Registers::zr);
                 return;
             }
@@ -3044,9 +3054,9 @@ private:
 
         move(TrustedImmPtr(address), memoryTempRegister);
         if (dest == memoryTempRegister)
-            m_cachedMemoryTempRegister.invalidate();
+            cachedMemoryTempRegister().invalidate();
         else
-            m_cachedMemoryTempRegister.setValue(reinterpret_cast<intptr_t>(address));
+            cachedMemoryTempRegister().setValue(reinterpret_cast<intptr_t>(address));
         m_assembler.ldr<datasize>(dest, memoryTempRegister, ARM64Registers::zr);
     }
 
@@ -3055,7 +3065,7 @@ private:
     {
         ASSERT(src != memoryTempRegister);
         intptr_t currentRegisterContents;
-        if (m_cachedMemoryTempRegister.value(currentRegisterContents)) {
+        if (cachedMemoryTempRegister().value(currentRegisterContents)) {
             intptr_t addressAsInt = reinterpret_cast<intptr_t>(address);
             intptr_t addressDelta = addressAsInt - currentRegisterContents;
 
@@ -3073,14 +3083,14 @@ private:
 
             if ((addressAsInt & (~maskHalfWord0)) == (currentRegisterContents & (~maskHalfWord0))) {
                 m_assembler.movk<64>(memoryTempRegister, addressAsInt & maskHalfWord0, 0);
-                m_cachedMemoryTempRegister.setValue(reinterpret_cast<intptr_t>(address));
+                cachedMemoryTempRegister().setValue(reinterpret_cast<intptr_t>(address));
                 m_assembler.str<datasize>(src, memoryTempRegister, ARM64Registers::zr);
                 return;
             }
         }
 
         move(TrustedImmPtr(address), memoryTempRegister);
-        m_cachedMemoryTempRegister.setValue(reinterpret_cast<intptr_t>(address));
+        cachedMemoryTempRegister().setValue(reinterpret_cast<intptr_t>(address));
         m_assembler.str<datasize>(src, memoryTempRegister, ARM64Registers::zr);
     }
 
index 4ad18b7..02cf3e4 100644 (file)
@@ -498,23 +498,23 @@ SignExtend32ToPtr U:G:32, D:G:Ptr
 
 ZeroExtend8To32 U:G:8, ZD:G:32
     Tmp, Tmp
-    Addr, Tmp as load8
-    Index, Tmp as load8
+    x86: Addr, Tmp as load8
+    x86: Index, Tmp as load8
 
 SignExtend8To32 U:G:8, ZD:G:32
     Tmp, Tmp
-    Addr, Tmp as load8SignedExtendTo32
-    Index, Tmp as load8SignedExtendTo32
+    x86: Addr, Tmp as load8SignedExtendTo32
+    x86: Index, Tmp as load8SignedExtendTo32
 
 ZeroExtend16To32 U:G:16, ZD:G:32
     Tmp, Tmp
-    Addr, Tmp as load16
-    Index, Tmp as load16
+    x86: Addr, Tmp as load16
+    x86: Index, Tmp as load16
 
 SignExtend16To32 U:G:16, ZD:G:32
     Tmp, Tmp
-    Addr, Tmp as load16SignedExtendTo32
-    Index, Tmp as load16SignedExtendTo32
+    x86: Addr, Tmp as load16SignedExtendTo32
+    x86: Index, Tmp as load16SignedExtendTo32
 
 MoveFloat U:F:32, D:F:32
     Tmp, Tmp as moveDouble