<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[230725] trunk</title>
</head>
<body>

<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }
#msg dl a { font-weight: bold}
#msg dl a:link    { color:#fc3; }
#msg dl a:active  { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/230725">230725</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2018-04-17 12:53:30 -0700 (Tue, 17 Apr 2018)</dd>
</dl>

<h3>Log Message</h3>
<pre>PutStackSinkingPhase should know that KillStack means ConflictingFlush
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):</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkJSTestsChangeLog">trunk/JSTests/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGClobberizeh">trunk/Source/JavaScriptCore/dfg/DFGClobberize.h</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGPutStackSinkingPhasecpp">trunk/Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreftlFTLLowerDFGToB3cpp">trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp</a></li>
</ul>

<h3>Added Paths</h3>
<ul>
<li><a href="#trunkJSTestsstresssinkputstackoverkillstackjs">trunk/JSTests/stress/sink-put-stack-over-kill-stack.js</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkJSTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/JSTests/ChangeLog (230724 => 230725)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/ChangeLog  2018-04-17 19:13:10 UTC (rev 230724)
+++ trunk/JSTests/ChangeLog     2018-04-17 19:53:30 UTC (rev 230725)
</span><span class="lines">@@ -1,3 +1,16 @@
</span><ins>+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):
+
</ins><span class="cx"> 2018-04-17  Yusuke Suzuki  <utatane.tea@gmail.com>
</span><span class="cx"> 
</span><span class="cx">         [JSC] Rename runWebAssembly to runWebAssemblySuite
</span></span></pre></div>
<a id="trunkJSTestsstresssinkputstackoverkillstackjs"></a>
<div class="addfile"><h4>Added: trunk/JSTests/stress/sink-put-stack-over-kill-stack.js (0 => 230725)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/stress/sink-put-stack-over-kill-stack.js                           (rev 0)
+++ trunk/JSTests/stress/sink-put-stack-over-kill-stack.js      2018-04-17 19:53:30 UTC (rev 230725)
</span><span class="lines">@@ -0,0 +1,16 @@
</span><ins>+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();
+}
</ins></span></pre></div>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (230724 => 230725)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog    2018-04-17 19:13:10 UTC (rev 230724)
+++ trunk/Source/JavaScriptCore/ChangeLog       2018-04-17 19:53:30 UTC (rev 230725)
</span><span class="lines">@@ -1,3 +1,37 @@
</span><ins>+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):
+
</ins><span class="cx"> 2018-04-17  Filip Pizlo  <fpizlo@apple.com>
</span><span class="cx"> 
</span><span class="cx">         JSWebAssemblyCodeBlock should be in an IsoSubspace
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGClobberizeh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGClobberize.h (230724 => 230725)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGClobberize.h  2018-04-17 19:13:10 UTC (rev 230724)
+++ trunk/Source/JavaScriptCore/dfg/DFGClobberize.h     2018-04-17 19:53:30 UTC (rev 230725)
</span><span class="lines">@@ -415,11 +415,14 @@
</span><span class="cx">     case ConstantStoragePointer:
</span><span class="cx">         def(PureValue(node, node->storagePointer()));
</span><span class="cx">         return;
</span><ins>+
+    case KillStack:
+        write(AbstractHeap(Stack, node->unlinkedLocal()));
+        return;
</ins><span class="cx">          
</span><span class="cx">     case MovHint:
</span><span class="cx">     case ZombieHint:
</span><span class="cx">     case ExitOK:
</span><del>-    case KillStack:
</del><span class="cx">     case Upsilon:
</span><span class="cx">     case Phi:
</span><span class="cx">     case PhantomLocal:
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGPutStackSinkingPhasecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp (230724 => 230725)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp      2018-04-17 19:13:10 UTC (rev 230724)
+++ trunk/Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp 2018-04-17 19:53:30 UTC (rev 230725)
</span><span class="lines">@@ -1,5 +1,5 @@
</span><span class="cx"> /*
</span><del>- * Copyright (C) 2014, 2015 Apple Inc. All rights reserved.
</del><ins>+ * Copyright (C) 2014-2018 Apple Inc. All rights reserved.
</ins><span class="cx">  *
</span><span class="cx">  * Redistribution and use in source and binary forms, with or without
</span><span class="cx">  * modification, are permitted provided that the following conditions
</span><span class="lines">@@ -123,7 +123,7 @@
</span><span class="cx">                     auto writeHandler = [&] (VirtualRegister operand) {
</span><span class="cx">                         if (operand.isHeader())
</span><span class="cx">                             return;
</span><del>-                        RELEASE_ASSERT(node->op() == PutStack || node->op() == LoadVarargs || node->op() == ForwardVarargs);
</del><ins>+                        RELEASE_ASSERT(node->op() == PutStack || node->op() == LoadVarargs || node->op() == ForwardVarargs || node->op() == KillStack);
</ins><span class="cx">                         writes.append(operand);
</span><span class="cx">                     };
</span><span class="cx"> 
</span><span class="lines">@@ -278,6 +278,10 @@
</span><span class="cx">                         VirtualRegister operand = node->stackAccessData()->local;
</span><span class="cx">                         deferred.operand(operand) = node->stackAccessData()->format;
</span><span class="cx">                         continue;
</span><ins>+                    } else if (node->op() == KillStack) {
+                        // We don't want to sink a PutStack past a KillStack.
+                        deferred.operand(node->unlinkedLocal()) = ConflictingFlush;
+                        continue;
</ins><span class="cx">                     }
</span><span class="cx">                     
</span><span class="cx">                     auto escapeHandler = [&] (VirtualRegister operand) {
</span><span class="lines">@@ -473,6 +477,11 @@
</span><span class="cx">                     node->convertToIdentity();
</span><span class="cx">                     break;
</span><span class="cx">                 }
</span><ins>+
+                case KillStack: {
+                    deferred.operand(node->unlinkedLocal()) = ConflictingFlush;
+                    break;
+                }
</ins><span class="cx">                 
</span><span class="cx">                 default: {
</span><span class="cx">                     auto escapeHandler = [&] (VirtualRegister operand) {
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreftlFTLLowerDFGToB3cpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp (230724 => 230725)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp      2018-04-17 19:13:10 UTC (rev 230724)
+++ trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp 2018-04-17 19:53:30 UTC (rev 230725)
</span><span class="lines">@@ -15871,7 +15871,7 @@
</span><span class="cx">                 Node* node = availability.node();
</span><span class="cx">                 if (!node->isPhantomAllocation())
</span><span class="cx">                     return;
</span><del>-                
</del><ins>+
</ins><span class="cx">                 auto result = map.add(node, nullptr);
</span><span class="cx">                 if (result.isNewEntry) {
</span><span class="cx">                     result.iterator->value =
</span><span class="lines">@@ -15899,6 +15899,8 @@
</span><span class="cx">         for (auto heapPair : availabilityMap.m_heap) {
</span><span class="cx">             Node* node = heapPair.key.base();
</span><span class="cx">             ExitTimeObjectMaterialization* materialization = map.get(node);
</span><ins>+            if (!materialization)
+                DFG_CRASH(m_graph, m_node, toCString("Could not find materialization for ", node, " in ", availabilityMap).data());
</ins><span class="cx">             ExitValue exitValue = exitValueForAvailability(arguments, map, heapPair.value);
</span><span class="cx">             if (exitValue.hasIndexInStackmapLocations())
</span><span class="cx">                 exitValue.adjustStackmapLocationsIndexByOffset(offsetOfExitArgumentsInStackmapLocations);
</span></span></pre>
</div>
</div>

</body>
</html>