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 792c567..f832bbd 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 f1b04b1..6bfd45a 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 dab456e..27e1435 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 fe010d6..a59e7f1 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 f9dce3b..1e20afb 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);