- Revision
- 251335
- Author
- bshaf...@apple.com
- Date
- 2019-10-20 10:32:43 -0700 (Sun, 20 Oct 2019)
Log Message
Cherry-pick r250585. rdar://problem/56280995
ObjectAllocationSinkingPhase shouldn't insert hints for allocations which are no longer valid
https://bugs.webkit.org/show_bug.cgi?id=199361
<rdar://problem/52454940>
Reviewed by Yusuke Suzuki.
JSTests:
* stress/allocation-sinking-hints-are-valid-ssa-2.js: Added.
(main.fn):
(main.executor):
(main):
* stress/allocation-sinking-hints-are-valid-ssa.js: Added.
(main.fn):
(main.executor):
(main):
Source/_javascript_Core:
In a prior fix to the object allocation sinking phase, I added code where we
made sure to insert PutHints over Phis for fields of an object at control flow
merge points. However, that code didn't consider that the base of the PutHint
may no longer be a valid heap location. This could cause us to emit invalid
SSA code by referring to a node which does not dominate the PutHint location.
This patch fixes the bug to only emit the PutHints when valid.
This patch also makes it so that DFGValidate actually validates that the graph
is in valid SSA form. E.g, any use of a node N must be dominated by N.
* dfg/DFGObjectAllocationSinkingPhase.cpp:
* dfg/DFGValidate.cpp:
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@250585 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Modified Paths
Added Paths
Diff
Modified: branches/safari-608-branch/JSTests/ChangeLog (251334 => 251335)
--- branches/safari-608-branch/JSTests/ChangeLog 2019-10-20 17:32:40 UTC (rev 251334)
+++ branches/safari-608-branch/JSTests/ChangeLog 2019-10-20 17:32:43 UTC (rev 251335)
@@ -1,5 +1,62 @@
2019-10-15 Kocsen Chung <kocsen_ch...@apple.com>
+ Cherry-pick r250585. rdar://problem/56280995
+
+ ObjectAllocationSinkingPhase shouldn't insert hints for allocations which are no longer valid
+ https://bugs.webkit.org/show_bug.cgi?id=199361
+ <rdar://problem/52454940>
+
+ Reviewed by Yusuke Suzuki.
+
+ JSTests:
+
+ * stress/allocation-sinking-hints-are-valid-ssa-2.js: Added.
+ (main.fn):
+ (main.executor):
+ (main):
+ * stress/allocation-sinking-hints-are-valid-ssa.js: Added.
+ (main.fn):
+ (main.executor):
+ (main):
+
+ Source/_javascript_Core:
+
+ In a prior fix to the object allocation sinking phase, I added code where we
+ made sure to insert PutHints over Phis for fields of an object at control flow
+ merge points. However, that code didn't consider that the base of the PutHint
+ may no longer be a valid heap location. This could cause us to emit invalid
+ SSA code by referring to a node which does not dominate the PutHint location.
+ This patch fixes the bug to only emit the PutHints when valid.
+
+ This patch also makes it so that DFGValidate actually validates that the graph
+ is in valid SSA form. E.g, any use of a node N must be dominated by N.
+
+ * dfg/DFGObjectAllocationSinkingPhase.cpp:
+ * dfg/DFGValidate.cpp:
+
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@250585 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2019-10-01 Saam Barati <sbar...@apple.com>
+
+ ObjectAllocationSinkingPhase shouldn't insert hints for allocations which are no longer valid
+ https://bugs.webkit.org/show_bug.cgi?id=199361
+ <rdar://problem/52454940>
+
+ Reviewed by Yusuke Suzuki.
+
+ * stress/allocation-sinking-hints-are-valid-ssa-2.js: Added.
+ (main.fn):
+ (main.executor):
+ (main):
+ * stress/allocation-sinking-hints-are-valid-ssa.js: Added.
+ (main.fn):
+ (main.executor):
+ (main):
+
+2019-10-15 Kocsen Chung <kocsen_ch...@apple.com>
+
Cherry-pick r249959. rdar://problem/56280989
CheckArray on DirectArguments/ScopedArguments does not filter out slow put array storage
Added: branches/safari-608-branch/JSTests/stress/allocation-sinking-hints-are-valid-ssa-2.js (0 => 251335)
--- branches/safari-608-branch/JSTests/stress/allocation-sinking-hints-are-valid-ssa-2.js (rev 0)
+++ branches/safari-608-branch/JSTests/stress/allocation-sinking-hints-are-valid-ssa-2.js 2019-10-20 17:32:43 UTC (rev 251335)
@@ -0,0 +1,31 @@
+function main() {
+ const arr = [0];
+ function executor(resolve, ...reject) {
+ arr;
+ if (resolve > arr) {
+ const fn = () => {
+ return fn;
+ };
+ for (const _ of arr) {
+ function fn() {}
+ arr.toString(arr, arr, arr, arr, arr, arr);
+ throw new Error();
+ }
+ } else {
+ for (const _ of [arr]) {
+ arr.toString();
+ }
+ const fn = () => {};
+ }
+ new Promise(executor, arr);
+ let some = {};
+ with(arr) {}
+ return reject;
+ }
+ executor();
+
+ for (let i = 0; i < 100; i++) {
+ executor();
+ }
+}
+main();
Added: branches/safari-608-branch/JSTests/stress/allocation-sinking-hints-are-valid-ssa.js (0 => 251335)
--- branches/safari-608-branch/JSTests/stress/allocation-sinking-hints-are-valid-ssa.js (rev 0)
+++ branches/safari-608-branch/JSTests/stress/allocation-sinking-hints-are-valid-ssa.js 2019-10-20 17:32:43 UTC (rev 251335)
@@ -0,0 +1,31 @@
+function main() {
+ const arr = [0];
+ function executor(resolve, ...reject) {
+ arr;
+ if (resolve > arr) {
+ const fn = () => {
+ return fn;
+ };
+ for (const _ of arr) {
+ function fn() {}
+ arr.toString(arr, arr, arr, arr, arr, arr);
+ throw new Error();
+ }
+ } else {
+ for (const _ of [arr]) {
+ arr.toString();
+ }
+ const fn = () => {};
+ }
+ new Promise(executor, arr);
+ let some = {};
+ with(arr) {}
+ return reject;
+ }
+ executor();
+
+ for (let i = 0; i < 100; i++) {
+ executor();
+ }
+}
+main();
Modified: branches/safari-608-branch/Source/_javascript_Core/ChangeLog (251334 => 251335)
--- branches/safari-608-branch/Source/_javascript_Core/ChangeLog 2019-10-20 17:32:40 UTC (rev 251334)
+++ branches/safari-608-branch/Source/_javascript_Core/ChangeLog 2019-10-20 17:32:43 UTC (rev 251335)
@@ -1,5 +1,66 @@
2019-10-15 Kocsen Chung <kocsen_ch...@apple.com>
+ Cherry-pick r250585. rdar://problem/56280995
+
+ ObjectAllocationSinkingPhase shouldn't insert hints for allocations which are no longer valid
+ https://bugs.webkit.org/show_bug.cgi?id=199361
+ <rdar://problem/52454940>
+
+ Reviewed by Yusuke Suzuki.
+
+ JSTests:
+
+ * stress/allocation-sinking-hints-are-valid-ssa-2.js: Added.
+ (main.fn):
+ (main.executor):
+ (main):
+ * stress/allocation-sinking-hints-are-valid-ssa.js: Added.
+ (main.fn):
+ (main.executor):
+ (main):
+
+ Source/_javascript_Core:
+
+ In a prior fix to the object allocation sinking phase, I added code where we
+ made sure to insert PutHints over Phis for fields of an object at control flow
+ merge points. However, that code didn't consider that the base of the PutHint
+ may no longer be a valid heap location. This could cause us to emit invalid
+ SSA code by referring to a node which does not dominate the PutHint location.
+ This patch fixes the bug to only emit the PutHints when valid.
+
+ This patch also makes it so that DFGValidate actually validates that the graph
+ is in valid SSA form. E.g, any use of a node N must be dominated by N.
+
+ * dfg/DFGObjectAllocationSinkingPhase.cpp:
+ * dfg/DFGValidate.cpp:
+
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@250585 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2019-10-01 Saam Barati <sbar...@apple.com>
+
+ ObjectAllocationSinkingPhase shouldn't insert hints for allocations which are no longer valid
+ https://bugs.webkit.org/show_bug.cgi?id=199361
+ <rdar://problem/52454940>
+
+ Reviewed by Yusuke Suzuki.
+
+ In a prior fix to the object allocation sinking phase, I added code where we
+ made sure to insert PutHints over Phis for fields of an object at control flow
+ merge points. However, that code didn't consider that the base of the PutHint
+ may no longer be a valid heap location. This could cause us to emit invalid
+ SSA code by referring to a node which does not dominate the PutHint location.
+ This patch fixes the bug to only emit the PutHints when valid.
+
+ This patch also makes it so that DFGValidate actually validates that the graph
+ is in valid SSA form. E.g, any use of a node N must be dominated by N.
+
+ * dfg/DFGObjectAllocationSinkingPhase.cpp:
+ * dfg/DFGValidate.cpp:
+
+2019-10-15 Kocsen Chung <kocsen_ch...@apple.com>
+
Cherry-pick r249959. rdar://problem/56280989
CheckArray on DirectArguments/ScopedArguments does not filter out slow put array storage
Modified: branches/safari-608-branch/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp (251334 => 251335)
--- branches/safari-608-branch/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp 2019-10-20 17:32:40 UTC (rev 251334)
+++ branches/safari-608-branch/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp 2019-10-20 17:32:43 UTC (rev 251335)
@@ -1815,9 +1815,11 @@
availabilityCalculator.m_availability, identifier, phiDef->value());
for (PromotedHeapLocation location : hintsForPhi[variable->index()]) {
- m_insertionSet.insert(0,
- location.createHint(m_graph, block->at(0)->origin.withInvalidExit(), phiDef->value()));
- m_localMapping.set(location, phiDef->value());
+ if (m_heap.onlyLocalAllocation(location.base())) {
+ m_insertionSet.insert(0,
+ location.createHint(m_graph, block->at(0)->origin.withInvalidExit(), phiDef->value()));
+ m_localMapping.set(location, phiDef->value());
+ }
}
}
Modified: branches/safari-608-branch/Source/_javascript_Core/dfg/DFGValidate.cpp (251334 => 251335)
--- branches/safari-608-branch/Source/_javascript_Core/dfg/DFGValidate.cpp 2019-10-20 17:32:40 UTC (rev 251334)
+++ branches/safari-608-branch/Source/_javascript_Core/dfg/DFGValidate.cpp 2019-10-20 17:32:43 UTC (rev 251335)
@@ -31,6 +31,7 @@
#include "CodeBlockWithJITType.h"
#include "DFGClobberize.h"
#include "DFGClobbersExitState.h"
+#include "DFGDominators.h"
#include "DFGMayExit.h"
#include "JSCInlines.h"
#include <wtf/Assertions.h>
@@ -775,6 +776,10 @@
VALIDATE((), !m_graph.m_argumentFormats.isEmpty()); // We always have at least one entrypoint.
VALIDATE((), m_graph.m_rootToArguments.isEmpty()); // This is only used in CPS.
+ m_graph.initializeNodeOwners();
+
+ auto& dominators = m_graph.ensureSSADominators();
+
for (unsigned entrypointIndex : m_graph.m_entrypointIndexToCatchBytecodeOffset.keys())
VALIDATE((), entrypointIndex > 0); // By convention, 0 is the entrypoint index for the op_enter entrypoint, which can not be in a catch.
@@ -788,6 +793,8 @@
bool didSeeExitOK = false;
bool isOSRExited = false;
+ HashSet<Node*> nodesInThisBlock;
+
for (auto* node : *block) {
didSeeExitOK |= node->origin.exitOK;
switch (node->op()) {
@@ -906,7 +913,13 @@
});
break;
}
+
isOSRExited |= node->isPseudoTerminal();
+
+ m_graph.doToChildren(node, [&] (Edge child) {
+ VALIDATE((node), dominators.strictlyDominates(child->owner, block) || nodesInThisBlock.contains(child.node()));
+ });
+ nodesInThisBlock.add(node);
}
}
}
