PutStackSinkingPhase should know that KillStack means ConflictingFlush
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Apr 2018 19:53:30 +0000 (19:53 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Apr 2018 19:53:30 +0000 (19:53 +0000)
https://bugs.webkit.org/show_bug.cgi?id=184672

Reviewed by Michael Saboff.

JSTests:

* stress/sink-put-stack-over-kill-stack.js: Added.
(avocado_1):
(apricot_0):
(__c_0):
(banana_2):

Source/JavaScriptCore:

We've had a long history of KillStack and PutStackSinkingPhase having problems. We kept changing the meaning of
KillStack, and at some point we removed reasoning about KillStack from PutStackSinkingPhase. I tried doing some
archeology - but I'm still not sure why that phase ignores KillStack entirely. Maybe it's an oversight or maybe it's
intentional - I don't know.

Whatever the history, it's clear from the attached test case that ignoring KillStack is not correct. The outcome of
doing so is that we will sometimes sink a PutStack below a KillStack. That's wrong because then, OSR exit will use
the value from the PutStack instead of using the value from the MovHint that is associated with the KillStack. So,
KillStack must be seen as a special kind of clobber of the stack slot. OSRAvailabiity uses ConflictingFlush. I think
that's correct here, too. If we used DeadFlush and that was merged with another control flow path that had a
specific flush format, then we would think that we could sink the flush from that path. That's not right, since that
could still lead to sinking a PutStack past the KillStack in the sense that a PutStack will appear after the
KillStack along one path through the CFG. Also, the definition of DeadFlush and ConflictingFlush in the comment
inside PutStackSinkingPhase seems to suggest that KillStack is a ConflictingFlush, since DeadFlush means that we
have done some PutStack and their values are still valid. KillStack is not a PutStack and it means that previous
values are not valid. The definition of ConflictingFlush is that "we know, via forward flow, that there isn't any
value in the given local that anyone should have been relying on" - which exactly matches KillStack's definition.

This also means that we cannot eliminate arguments allocations that are live over KillStacks, since if we eliminated
them then we would have a GetStack after a KillStack. One easy way to fix this is to say that KillStack writes to
its stack slot for the purpose of clobberize.

* dfg/DFGClobberize.h: KillStack "writes" to its stack slot.
* dfg/DFGPutStackSinkingPhase.cpp: Fix the bug.
* ftl/FTLLowerDFGToB3.cpp: Add better assertion failure.
(JSC::FTL::DFG::LowerDFGToB3::buildExitArguments):

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

JSTests/ChangeLog
JSTests/stress/sink-put-stack-over-kill-stack.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGClobberize.h
Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

index 792c5678bf88b39071cbcb9f5cdba0202ea6dd3d..f832bbd70f3de24e5c02990a8d077a499e7126e9 100644 (file)
@@ -1,3 +1,16 @@
+2018-04-16  Filip Pizlo  <fpizlo@apple.com>
+
+        PutStackSinkingPhase should know that KillStack means ConflictingFlush
+        https://bugs.webkit.org/show_bug.cgi?id=184672
+
+        Reviewed by Michael Saboff.
+
+        * stress/sink-put-stack-over-kill-stack.js: Added.
+        (avocado_1):
+        (apricot_0):
+        (__c_0):
+        (banana_2):
+
 2018-04-17  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [JSC] Rename runWebAssembly to runWebAssemblySuite
diff --git a/JSTests/stress/sink-put-stack-over-kill-stack.js b/JSTests/stress/sink-put-stack-over-kill-stack.js
new file mode 100644 (file)
index 0000000..151a07d
--- /dev/null
@@ -0,0 +1,16 @@
+function* avocado_1() {}
+
+function apricot_0(alpaca_0) {
+  if (alpaca_0) {}
+}
+
+class __c_0 extends null {}
+
+function banana_2() {
+  apricot_0();
+  avocado_1(() => null);
+}
+
+for (let i = 0; i < 100000; i++) {
+  banana_2();
+}
index f1b04b15b25e3ed8f9ed7c5c5e8087808561e9de..6bfd45a72796d9f440b6eed74d45017e286e28f2 100644 (file)
@@ -1,3 +1,37 @@
+2018-04-16  Filip Pizlo  <fpizlo@apple.com>
+
+        PutStackSinkingPhase should know that KillStack means ConflictingFlush
+        https://bugs.webkit.org/show_bug.cgi?id=184672
+
+        Reviewed by Michael Saboff.
+
+        We've had a long history of KillStack and PutStackSinkingPhase having problems. We kept changing the meaning of
+        KillStack, and at some point we removed reasoning about KillStack from PutStackSinkingPhase. I tried doing some
+        archeology - but I'm still not sure why that phase ignores KillStack entirely. Maybe it's an oversight or maybe it's
+        intentional - I don't know.
+
+        Whatever the history, it's clear from the attached test case that ignoring KillStack is not correct. The outcome of
+        doing so is that we will sometimes sink a PutStack below a KillStack. That's wrong because then, OSR exit will use
+        the value from the PutStack instead of using the value from the MovHint that is associated with the KillStack. So,
+        KillStack must be seen as a special kind of clobber of the stack slot. OSRAvailabiity uses ConflictingFlush. I think
+        that's correct here, too. If we used DeadFlush and that was merged with another control flow path that had a
+        specific flush format, then we would think that we could sink the flush from that path. That's not right, since that
+        could still lead to sinking a PutStack past the KillStack in the sense that a PutStack will appear after the
+        KillStack along one path through the CFG. Also, the definition of DeadFlush and ConflictingFlush in the comment
+        inside PutStackSinkingPhase seems to suggest that KillStack is a ConflictingFlush, since DeadFlush means that we
+        have done some PutStack and their values are still valid. KillStack is not a PutStack and it means that previous
+        values are not valid. The definition of ConflictingFlush is that "we know, via forward flow, that there isn't any
+        value in the given local that anyone should have been relying on" - which exactly matches KillStack's definition.
+
+        This also means that we cannot eliminate arguments allocations that are live over KillStacks, since if we eliminated
+        them then we would have a GetStack after a KillStack. One easy way to fix this is to say that KillStack writes to
+        its stack slot for the purpose of clobberize.
+
+        * dfg/DFGClobberize.h: KillStack "writes" to its stack slot.
+        * dfg/DFGPutStackSinkingPhase.cpp: Fix the bug.
+        * ftl/FTLLowerDFGToB3.cpp: Add better assertion failure.
+        (JSC::FTL::DFG::LowerDFGToB3::buildExitArguments):
+
 2018-04-17  Filip Pizlo  <fpizlo@apple.com>
 
         JSWebAssemblyCodeBlock should be in an IsoSubspace
index dab456ed156761c35342890cb42d333eef3df154..27e14354f711965edb4bfbcc92e4d5c24c6b0d45 100644 (file)
@@ -415,11 +415,14 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
     case ConstantStoragePointer:
         def(PureValue(node, node->storagePointer()));
         return;
+
+    case KillStack:
+        write(AbstractHeap(Stack, node->unlinkedLocal()));
+        return;
          
     case MovHint:
     case ZombieHint:
     case ExitOK:
-    case KillStack:
     case Upsilon:
     case Phi:
     case PhantomLocal:
index fe010d63fd90ed8686ce762d6e662064f58bde92..a59e7f165b985809becf9e90709a759dc946916a 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014, 2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2014-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -123,7 +123,7 @@ public:
                     auto writeHandler = [&] (VirtualRegister operand) {
                         if (operand.isHeader())
                             return;
-                        RELEASE_ASSERT(node->op() == PutStack || node->op() == LoadVarargs || node->op() == ForwardVarargs);
+                        RELEASE_ASSERT(node->op() == PutStack || node->op() == LoadVarargs || node->op() == ForwardVarargs || node->op() == KillStack);
                         writes.append(operand);
                     };
 
@@ -278,6 +278,10 @@ public:
                         VirtualRegister operand = node->stackAccessData()->local;
                         deferred.operand(operand) = node->stackAccessData()->format;
                         continue;
+                    } else if (node->op() == KillStack) {
+                        // We don't want to sink a PutStack past a KillStack.
+                        deferred.operand(node->unlinkedLocal()) = ConflictingFlush;
+                        continue;
                     }
                     
                     auto escapeHandler = [&] (VirtualRegister operand) {
@@ -473,6 +477,11 @@ public:
                     node->convertToIdentity();
                     break;
                 }
+
+                case KillStack: {
+                    deferred.operand(node->unlinkedLocal()) = ConflictingFlush;
+                    break;
+                }
                 
                 default: {
                     auto escapeHandler = [&] (VirtualRegister operand) {
index f9dce3b0fcd20dfd951ad6b91f77fffbd0075622..1e20afbdba912475472e86cb98ef8beab663eb8b 100644 (file)
@@ -15871,7 +15871,7 @@ private:
                 Node* node = availability.node();
                 if (!node->isPhantomAllocation())
                     return;
-                
+
                 auto result = map.add(node, nullptr);
                 if (result.isNewEntry) {
                     result.iterator->value =
@@ -15899,6 +15899,8 @@ private:
         for (auto heapPair : availabilityMap.m_heap) {
             Node* node = heapPair.key.base();
             ExitTimeObjectMaterialization* materialization = map.get(node);
+            if (!materialization)
+                DFG_CRASH(m_graph, m_node, toCString("Could not find materialization for ", node, " in ", availabilityMap).data());
             ExitValue exitValue = exitValueForAvailability(arguments, map, heapPair.value);
             if (exitValue.hasIndexInStackmapLocations())
                 exitValue.adjustStackmapLocationsIndexByOffset(offsetOfExitArgumentsInStackmapLocations);