Title: [278937] trunk
Revision
278937
Author
tzaga...@apple.com
Date
2021-06-16 09:09:24 -0700 (Wed, 16 Jun 2021)

Log Message

AssemblyHelpers should save/restore callee save FPRs
https://bugs.webkit.org/show_bug.cgi?id=227052
<rdar://77080162>

Reviewed by Mark Lam.

JSTests:

* stress/callee-save-fpr.js: Added.
(_f):
(_g):
(_h):
(_i):
(assertEqual):

Source/_javascript_Core:

We have 3 functions in AssemblyHelpers to save and restore callee save registers that were filtering
out any FPRs. This is an issue since we do have callee save FPRs in arm64 and these functions can be
called from the FTL, and FTL uses those callee saves. The test case shows how that's an issue with tail
calls on FTL: the callee saves are correctly stored in the prologue and restored in the epilogue, but
when emitting a tail call we use AssemblyHelpers::emitRestoreCalleeSaves to restore the callee saves,
which doesn't restore FPRs. This results in the callee save FPRs being trashed. To fix this we just need
to stop filtering out the FPRs, if they are listed as used by the code block they should be saved/restored
accordingly. I also changed DFGOSREntry to stop filtering out the callee save FPRs and instead assert
there aren't any, since they aren't currently used in the DFG, but it could help avoid the same issue in
the future.

* dfg/DFGOSREntry.cpp:
(JSC::DFG::prepareOSREntry):
* jit/AssemblyHelpers.h:
(JSC::AssemblyHelpers::emitSaveCalleeSavesFor):
(JSC::AssemblyHelpers::emitSaveOrCopyCalleeSavesFor):
(JSC::AssemblyHelpers::emitRestoreCalleeSavesFor):

Modified Paths

Added Paths

Diff

Modified: trunk/JSTests/ChangeLog (278936 => 278937)


--- trunk/JSTests/ChangeLog	2021-06-16 15:41:15 UTC (rev 278936)
+++ trunk/JSTests/ChangeLog	2021-06-16 16:09:24 UTC (rev 278937)
@@ -1,3 +1,18 @@
+2021-06-16  Tadeu Zagallo  <tzaga...@apple.com>
+
+        AssemblyHelpers should save/restore callee save FPRs
+        https://bugs.webkit.org/show_bug.cgi?id=227052
+        <rdar://77080162>
+
+        Reviewed by Mark Lam.
+
+        * stress/callee-save-fpr.js: Added.
+        (_f):
+        (_g):
+        (_h):
+        (_i):
+        (assertEqual):
+
 2021-06-15  Yusuke Suzuki  <ysuz...@apple.com>
 
         [JSC] Optimize JSON.parse with small content by dropping single character Identifier pool

Added: trunk/JSTests/stress/callee-save-fpr.js (0 => 278937)


--- trunk/JSTests/stress/callee-save-fpr.js	                        (rev 0)
+++ trunk/JSTests/stress/callee-save-fpr.js	2021-06-16 16:09:24 UTC (rev 278937)
@@ -0,0 +1,103 @@
+'use strict';
+
+function _f(a1, a2, a, b, c, d, e, f, g, h, i, j, k, l, m, n, o) {
+    a *= 1.1;
+    b *= 1.2;
+    c *= 1.3;
+    d *= 1.4;
+    e *= 1.5;
+    f *= 1.6;
+    g *= 1.7;
+    h *= 1.8;
+    i *= 1.9;
+    j *= 2.1;
+    k *= 2.2;
+    l *= 2.3;
+    m *= 2.4;
+    n *= 2.5;
+    o *= 2.6;
+
+    a1[0] = a;
+    a1[1] = b;
+    a1[2] = c;
+    a1[3] = d;
+    a1[4] = e;
+    a1[5] = f;
+    a1[6] = g;
+    a1[7] = h;
+    a1[8] = i;
+    a1[9] = j;
+    a1[10] = k;
+    a1[11] = l;
+    a1[12] = m;
+    a1[13] = n;
+    a1[14] = o;
+
+    _g(a1, a, b, c, d, e, f, g, h, i, j, k, l, m, n, o);
+
+    a2[0] = a;
+    a2[1] = b;
+    a2[2] = c;
+    a2[3] = d;
+    a2[4] = e;
+    a2[5] = f;
+    a2[6] = g;
+    a2[7] = h;
+    a2[8] = i;
+    a2[9] = j;
+    a2[10] = k;
+    a2[11] = l;
+    a2[12] = m;
+    a2[13] = n;
+    a2[14] = o;
+}
+noInline(_f);
+
+function _g(x, a, b, c, d, e, f, g, h, i, j, k, l, m, n, o) {
+    a *= 1.1;
+    b *= 1.2;
+    c *= 1.3;
+    d *= 1.4;
+    e *= 1.5;
+    f *= 1.6;
+    g *= 1.7;
+    h *= 1.8;
+    i *= 1.9;
+    j *= 2.1;
+    k *= 2.2;
+    l *= 2.3;
+    m *= 2.4;
+    n *= 2.5;
+    o *= 2.6;
+
+    x[15] = a + b + c + d + e + f + g + h + i + j + k + l + m + n + o;
+    _i(x);
+    return _h(x, ...[a, b, c, d, e, f, g, h, i, j, k, l, m, n, o]);
+}
+noInline(_g);
+
+function _h(x, a, b, c, d, e, f, g, h, i, j, k, l, m, n, o) {
+}
+noInline(_h);
+
+function _i() { }
+noInline(_i);
+
+function assertEqual(x, y) {
+    if (x !== y)
+        throw new Error(`assertEqual: fail: ${x} !== ${y}`);
+}
+noInline(assertEqual);
+
+const count = 15;
+let args = [];
+for (let i = 1; i <= count; ++i)
+    args.push(i);
+
+for (let i = 0; i < 1e5; ++i) {
+    let a1 = new Float64Array(count);
+    let a2 = new Float64Array(count);
+    _f(a1, a2, ...args);
+    for (let j = 0; j < count; ++j)
+        assertEqual(a1[j], a2[j]);
+}

Modified: trunk/Source/_javascript_Core/ChangeLog (278936 => 278937)


--- trunk/Source/_javascript_Core/ChangeLog	2021-06-16 15:41:15 UTC (rev 278936)
+++ trunk/Source/_javascript_Core/ChangeLog	2021-06-16 16:09:24 UTC (rev 278937)
@@ -1,3 +1,29 @@
+2021-06-16  Tadeu Zagallo  <tzaga...@apple.com>
+
+        AssemblyHelpers should save/restore callee save FPRs
+        https://bugs.webkit.org/show_bug.cgi?id=227052
+        <rdar://77080162>
+
+        Reviewed by Mark Lam.
+
+        We have 3 functions in AssemblyHelpers to save and restore callee save registers that were filtering
+        out any FPRs. This is an issue since we do have callee save FPRs in arm64 and these functions can be
+        called from the FTL, and FTL uses those callee saves. The test case shows how that's an issue with tail
+        calls on FTL: the callee saves are correctly stored in the prologue and restored in the epilogue, but
+        when emitting a tail call we use AssemblyHelpers::emitRestoreCalleeSaves to restore the callee saves,
+        which doesn't restore FPRs. This results in the callee save FPRs being trashed. To fix this we just need
+        to stop filtering out the FPRs, if they are listed as used by the code block they should be saved/restored
+        accordingly. I also changed DFGOSREntry to stop filtering out the callee save FPRs and instead assert
+        there aren't any, since they aren't currently used in the DFG, but it could help avoid the same issue in
+        the future.
+
+        * dfg/DFGOSREntry.cpp:
+        (JSC::DFG::prepareOSREntry):
+        * jit/AssemblyHelpers.h:
+        (JSC::AssemblyHelpers::emitSaveCalleeSavesFor):
+        (JSC::AssemblyHelpers::emitSaveOrCopyCalleeSavesFor):
+        (JSC::AssemblyHelpers::emitRestoreCalleeSavesFor):
+
 2021-06-16  Commit Queue  <commit-qu...@webkit.org>
 
         Unreviewed, reverting r278846.

Modified: trunk/Source/_javascript_Core/dfg/DFGOSREntry.cpp (278936 => 278937)


--- trunk/Source/_javascript_Core/dfg/DFGOSREntry.cpp	2021-06-16 15:41:15 UTC (rev 278936)
+++ trunk/Source/_javascript_Core/dfg/DFGOSREntry.cpp	2021-06-16 16:09:24 UTC (rev 278937)
@@ -297,7 +297,7 @@
 #if NUMBER_OF_CALLEE_SAVES_REGISTERS > 0
     const RegisterAtOffsetList* registerSaveLocations = codeBlock->calleeSaveRegisters();
     RegisterAtOffsetList* allCalleeSaves = RegisterSet::vmCalleeSaveRegisterOffsets();
-    RegisterSet dontSaveRegisters = RegisterSet(RegisterSet::stackRegisters(), RegisterSet::allFPRs());
+    RegisterSet dontSaveRegisters = RegisterSet(RegisterSet::stackRegisters());
 
     unsigned registerCount = registerSaveLocations->size();
     VMEntryRecord* record = vmEntryRecord(vm.topEntryFrame);
@@ -305,6 +305,7 @@
         RegisterAtOffset currentEntry = registerSaveLocations->at(i);
         if (dontSaveRegisters.get(currentEntry.reg()))
             continue;
+        RELEASE_ASSERT(currentEntry.reg().isGPR());
         RegisterAtOffset* calleeSavesEntry = allCalleeSaves->find(currentEntry.reg());
         
         if constexpr (CallerFrameAndPC::sizeInRegisters == 2)

Modified: trunk/Source/_javascript_Core/jit/AssemblyHelpers.h (278936 => 278937)


--- trunk/Source/_javascript_Core/jit/AssemblyHelpers.h	2021-06-16 15:41:15 UTC (rev 278936)
+++ trunk/Source/_javascript_Core/jit/AssemblyHelpers.h	2021-06-16 16:09:24 UTC (rev 278937)
@@ -331,7 +331,7 @@
 
     void emitSaveCalleeSavesFor(const RegisterAtOffsetList* calleeSaves)
     {
-        RegisterSet dontSaveRegisters = RegisterSet(RegisterSet::stackRegisters(), RegisterSet::allFPRs());
+        RegisterSet dontSaveRegisters = RegisterSet(RegisterSet::stackRegisters());
         unsigned registerCount = calleeSaves->size();
 
         for (unsigned i = 0; i < registerCount; i++) {
@@ -338,7 +338,7 @@
             RegisterAtOffset entry = calleeSaves->at(i);
             if (dontSaveRegisters.get(entry.reg()))
                 continue;
-            storePtr(entry.reg().gpr(), Address(framePointerRegister, entry.offset()));
+            storeReg(entry.reg(), Address(framePointerRegister, entry.offset()));
         }
     }
     
@@ -347,9 +347,10 @@
     void emitSaveOrCopyCalleeSavesFor(CodeBlock* codeBlock, VirtualRegister offsetVirtualRegister, RestoreTagRegisterMode tagRegisterMode, GPRReg temp)
     {
         ASSERT(codeBlock);
+        ASSERT(JITCode::isBaselineCode(codeBlock->jitType()));
         
         const RegisterAtOffsetList* calleeSaves = codeBlock->calleeSaveRegisters();
-        RegisterSet dontSaveRegisters = RegisterSet(RegisterSet::stackRegisters(), RegisterSet::allFPRs());
+        RegisterSet dontSaveRegisters = RegisterSet(RegisterSet::stackRegisters());
         unsigned registerCount = calleeSaves->size();
 
 #if USE(JSVALUE64)
@@ -360,9 +361,10 @@
             RegisterAtOffset entry = calleeSaves->at(i);
             if (dontSaveRegisters.get(entry.reg()))
                 continue;
-            
+            RELEASE_ASSERT(entry.reg().isGPR());
+
             GPRReg registerToWrite;
-            
+
 #if USE(JSVALUE32_64)
             UNUSED_PARAM(tagRegisterMode);
             UNUSED_PARAM(temp);
@@ -388,7 +390,7 @@
 
     void emitRestoreCalleeSavesFor(const RegisterAtOffsetList* calleeSaves)
     {
-        RegisterSet dontRestoreRegisters = RegisterSet(RegisterSet::stackRegisters(), RegisterSet::allFPRs());
+        RegisterSet dontRestoreRegisters = RegisterSet(RegisterSet::stackRegisters());
         unsigned registerCount = calleeSaves->size();
         
         for (unsigned i = 0; i < registerCount; i++) {
@@ -395,7 +397,7 @@
             RegisterAtOffset entry = calleeSaves->at(i);
             if (dontRestoreRegisters.get(entry.reg()))
                 continue;
-            loadPtr(Address(framePointerRegister, entry.offset()), entry.reg().gpr());
+            loadReg(Address(framePointerRegister, entry.offset()), entry.reg());
         }
     }
 
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to