Changeset 192353 in webkit

Timestamp:

Nov 11, 2015 11:17:09 PM (8 years ago)

Author:

mark.lam@apple.com

Message:

The BinarySnippetRegisterContext needs to copy the result back from the scratch register.
​https://bugs.webkit.org/show_bug.cgi?id=150712

Reviewed by Geoffrey Garen.

If the BinarySnippetRegisterContext had re-assigned the result register to a scratch
before emitting the snippet, it needs to copy the result back from the scratch after
the snippet is done.

This fixes the cdjs-tests.yaml/main.js.ftl failure reported in
​https://bugs.webkit.org/show_bug.cgi?id=150687.

Also added an optimization to check for the case where any of the left, right,
or result registers are aliased together, and to map them to the corresponding
allocated scratch register for their alias instead of allocating separate ones.

• JavaScriptCore.vcxproj/JavaScriptCore.vcxproj:
• JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters:
• Added JITSubGenerator.h to these project files for completeness.

• ftl/FTLCapabilities.cpp:

(JSC::FTL::canCompile):

• Re-enable ArithSub handling of UntypedUse operands.

• ftl/FTLCompile.cpp:

• ftl/FTLInlineCacheSize.cpp:

(JSC::FTL::sizeOfArithSub):

• Adjusted IC sizes to account for the snippet changes.

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• JavaScriptCore.vcxproj/JavaScriptCore.vcxproj (modified) (1 diff)
• JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters (modified) (1 diff)
• ftl/FTLCapabilities.cpp (modified) (2 diffs)
• ftl/FTLCompile.cpp (modified) (5 diffs)
• ftl/FTLInlineCacheSize.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2015-11-11  Mark Lam  <mark.lam@apple.com>
+
+        The BinarySnippetRegisterContext needs to copy the result back from the scratch register.
+        https://bugs.webkit.org/show_bug.cgi?id=150712
+
+        Reviewed by Geoffrey Garen.
+
+        If the BinarySnippetRegisterContext had re-assigned the result register to a scratch
+        before emitting the snippet, it needs to copy the result back from the scratch after
+        the snippet is done.
+
+        This fixes the cdjs-tests.yaml/main.js.ftl failure reported in
+        https://bugs.webkit.org/show_bug.cgi?id=150687.
+
+        Also added an optimization to check for the case where any of the left, right,
+        or result registers are aliased together, and to map them to the corresponding
+        allocated scratch register for their alias instead of allocating separate ones.
+
+        * JavaScriptCore.vcxproj/JavaScriptCore.vcxproj:
+        * JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters:
+        - Added JITSubGenerator.h to these project files for completeness.
+
+        * ftl/FTLCapabilities.cpp:
+        (JSC::FTL::canCompile):
+        - Re-enable ArithSub handling of UntypedUse operands.
+
+        * ftl/FTLCompile.cpp:
+
+        * ftl/FTLInlineCacheSize.cpp:
+        (JSC::FTL::sizeOfArithSub):
+        - Adjusted IC sizes to account for the snippet changes.
+
 2015-11-11  Mark Lam  <mark.lam@apple.com>
 

trunk/Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj

@@ -1470 +1470 @@
     <ClInclude Include="..\jit\JITOperations.h" />
     <ClInclude Include="..\jit\JITStubRoutine.h" />
+    <ClInclude Include="..\jit\JITSubGenerator.h" />
     <ClInclude Include="..\jit\JITThunks.h" />
     <ClInclude Include="..\jit\JITToDFGDeferredCompilationCallback.h" />

trunk/Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters

@@ -2523 +2523 @@
       <Filter>jit</Filter>
     </ClInclude>
+    <ClInclude Include="..\jit\JITSubGenerator.h">
+      <Filter>jit</Filter>
+    </ClInclude>
     <ClInclude Include="..\jit\JITThunks.h">
       <Filter>jit</Filter>

trunk/Source/JavaScriptCore/ftl/FTLCapabilities.cpp

@@ -85 +85 @@
     case ArithAdd:
     case ArithClz32:
+    case ArithSub:
     case ArithMul:
     case ArithDiv:
@@ -212 +213 @@
         // These are OK.
         break;
-    case ArithSub:
-        if (node->result() == NodeResultJS)
-            return CannotCompile;
-        break;
 
     case Identity:

trunk/Source/JavaScriptCore/ftl/FTLCompile.cpp

@@ -311 +311 @@
     // The purpose of this class is to shuffle registers to get them into the state
     // that baseline code expects so that we can use the baseline snippet generators i.e.
-    //    1. ensure that the inputs and outputs are not in tag or scratch registers.
-    //    2. tag registers are loaded with the expected values.
+    //    1. Ensure that the inputs and output are not in reserved registers (which
+    //       include the tag registers). The snippet will use these reserved registers.
+    //       Hence, we need to put the inputs and output in other scratch registers.
+    //    2. Tag registers are loaded with the expected values.
     //
-    // We also need to:
-    //    1. restore the input and tag registers to the values that LLVM put there originally.
-    //    2. that is except when one of the input registers is also the result register.
-    //       In this case, we don't want to trash the result, and hence, should not restore into it.
+    // When the snippet is done:
+    //    1. If we had re-assigned the result register to a scratch, we need to copy the
+    //       result back from the scratch.
+    //    2. Restore the input and tag registers to the values that LLVM put there originally.
+    //       That is unless when one of them is also the result register. In that case, we
+    //       don't want to trash the result, and hence, should not restore into it.
 
 public:
@@ -333 +337 @@
         m_allocator.lock(m_right);
 
-        RegisterSet inputRegisters = RegisterSet(m_left, m_right);
-        RegisterSet inputAndOutputRegisters = RegisterSet(inputRegisters, m_result);
-
+        RegisterSet inputAndOutputRegisters = RegisterSet(m_left, m_right, m_result);
         RegisterSet reservedRegisters;
         for (GPRReg reg : GPRInfo::reservedRegisters())
@@ -342 +344 @@
         if (reservedRegisters.get(m_left))
             m_left = m_allocator.allocateScratchGPR();
-        if (reservedRegisters.get(m_right))
-            m_right = m_allocator.allocateScratchGPR();
-        if (!inputRegisters.get(m_result) && reservedRegisters.get(m_result))
-            m_result = m_allocator.allocateScratchGPR();
-        
+        if (reservedRegisters.get(m_right)) {
+            if (m_origRight == m_origLeft)
+                m_right = m_left;
+            else
+                m_right = m_allocator.allocateScratchGPR();
+        }
+        if (reservedRegisters.get(m_result)) {
+            if (m_origResult == m_origLeft)
+                m_result = m_left;
+            else if (m_origResult == m_origRight)
+                m_result = m_right;
+            else
+                m_result = m_allocator.allocateScratchGPR();
+        }
+
         if (!inputAndOutputRegisters.get(GPRInfo::tagMaskRegister))
             m_savedTagMaskRegister = m_allocator.allocateScratchGPR();
@@ -357 +369 @@
         if (m_left != m_origLeft)
             jit.move(m_origLeft, m_left);
-        if (m_right != m_origRight)
+        if (m_right != m_origRight && m_origRight != m_origLeft)
             jit.move(m_origRight, m_right);
 
@@ -370 +382 @@
     void restoreRegisters(CCallHelpers& jit)
     {
+        if (m_origResult != m_result)
+            jit.move(m_result, m_origResult);
         if (m_origLeft != m_left && m_origLeft != m_origResult)
             jit.move(m_left, m_origLeft);
-        if (m_origRight != m_right && m_origRight != m_origResult)
+        if (m_origRight != m_right && m_origRight != m_origResult && m_origRight != m_origLeft)
             jit.move(m_right, m_origRight);
-        
-        if (m_savedTagMaskRegister != InvalidGPRReg)
+
+        // We are guaranteed that the tag registers are not the same as the original input
+        // or output registers. Otherwise, we would not have allocated a scratch for them.
+        // Hence, we don't need to need to check for overlap like we do for the input registers.
+        if (m_savedTagMaskRegister != InvalidGPRReg) {
+            ASSERT(GPRInfo::tagMaskRegister != m_origLeft);
+            ASSERT(GPRInfo::tagMaskRegister != m_origRight);
+            ASSERT(GPRInfo::tagMaskRegister != m_origResult);
             jit.move(m_savedTagMaskRegister, GPRInfo::tagMaskRegister);
-        if (m_savedTagTypeNumberRegister != InvalidGPRReg)
+        }
+        if (m_savedTagTypeNumberRegister != InvalidGPRReg) {
+            ASSERT(GPRInfo::tagTypeNumberRegister != m_origLeft);
+            ASSERT(GPRInfo::tagTypeNumberRegister != m_origRight);
+            ASSERT(GPRInfo::tagTypeNumberRegister != m_origResult);
             jit.move(m_savedTagTypeNumberRegister, GPRInfo::tagTypeNumberRegister);
+        }
     }
 

trunk/Source/JavaScriptCore/ftl/FTLInlineCacheSize.cpp

@@ -133 +133 @@
 #if CPU(ARM64)
 #ifdef NDEBUG
-    return 192; // ARM64 release.
+    return 216; // ARM64 release.
 #else
-    return 288; // ARM64 debug.
+    return 312; // ARM64 debug.
 #endif
 #else // CPU(X86_64)
 #ifdef NDEBUG
-    return 184; // X86_64 release.
+    return 223; // X86_64 release.
 #else
-    return 259; // X86_64 debug.
+    return 298; // X86_64 debug.
 #endif
 #endif