- Revision
- 230725
- Author
- fpi...@apple.com
- Date
- 2018-04-17 12:53:30 -0700 (Tue, 17 Apr 2018)
Log Message
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/_javascript_Core:
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):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (230724 => 230725)
--- trunk/JSTests/ChangeLog 2018-04-17 19:13:10 UTC (rev 230724)
+++ trunk/JSTests/ChangeLog 2018-04-17 19:53:30 UTC (rev 230725)
@@ -1,3 +1,16 @@
+2018-04-16 Filip Pizlo <fpi...@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....@gmail.com>
[JSC] Rename runWebAssembly to runWebAssemblySuite
Added: trunk/JSTests/stress/sink-put-stack-over-kill-stack.js (0 => 230725)
--- 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)
@@ -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();
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (230724 => 230725)
--- trunk/Source/_javascript_Core/ChangeLog 2018-04-17 19:13:10 UTC (rev 230724)
+++ trunk/Source/_javascript_Core/ChangeLog 2018-04-17 19:53:30 UTC (rev 230725)
@@ -1,3 +1,37 @@
+2018-04-16 Filip Pizlo <fpi...@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 <fpi...@apple.com>
JSWebAssemblyCodeBlock should be in an IsoSubspace
Modified: trunk/Source/_javascript_Core/dfg/DFGClobberize.h (230724 => 230725)
--- trunk/Source/_javascript_Core/dfg/DFGClobberize.h 2018-04-17 19:13:10 UTC (rev 230724)
+++ trunk/Source/_javascript_Core/dfg/DFGClobberize.h 2018-04-17 19:53:30 UTC (rev 230725)
@@ -415,11 +415,14 @@
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:
Modified: trunk/Source/_javascript_Core/dfg/DFGPutStackSinkingPhase.cpp (230724 => 230725)
--- trunk/Source/_javascript_Core/dfg/DFGPutStackSinkingPhase.cpp 2018-04-17 19:13:10 UTC (rev 230724)
+++ trunk/Source/_javascript_Core/dfg/DFGPutStackSinkingPhase.cpp 2018-04-17 19:53:30 UTC (rev 230725)
@@ -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 @@
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 @@
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 @@
node->convertToIdentity();
break;
}
+
+ case KillStack: {
+ deferred.operand(node->unlinkedLocal()) = ConflictingFlush;
+ break;
+ }
default: {
auto escapeHandler = [&] (VirtualRegister operand) {
Modified: trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp (230724 => 230725)
--- trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2018-04-17 19:13:10 UTC (rev 230724)
+++ trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2018-04-17 19:53:30 UTC (rev 230725)
@@ -15871,7 +15871,7 @@
Node* node = availability.node();
if (!node->isPhantomAllocation())
return;
-
+
auto result = map.add(node, nullptr);
if (result.isNewEntry) {
result.iterator->value =
@@ -15899,6 +15899,8 @@
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);
