- Revision
- 222060
- Author
- sbar...@apple.com
- Date
- 2017-09-14 16:39:27 -0700 (Thu, 14 Sep 2017)
Log Message
It should be valid to exit before each set when doing arity fixup when inlining
https://bugs.webkit.org/show_bug.cgi?id=176948
Reviewed by Keith Miller.
JSTests:
* stress/arity-fixup-inlining-dont-generate-invalid-use.js: Added.
(baz):
(bar):
(foo):
Source/_javascript_Core:
This patch makes it so that we can exit before each SetLocal when doing arity
fixup during inlining. This is OK because if we exit at any of these SetLocals,
we will simply exit to the beginning of the call instruction.
Not doing this led to a bug where FixupPhase would insert a ValueRep of
a node before the actual node. This is obviously invalid IR. I've added
a new validation rule to catch this malformed IR.
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::inliningCost):
(JSC::DFG::ByteCodeParser::inlineCall):
* dfg/DFGValidate.cpp:
* runtime/Options.h:
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (222059 => 222060)
--- trunk/JSTests/ChangeLog 2017-09-14 23:37:32 UTC (rev 222059)
+++ trunk/JSTests/ChangeLog 2017-09-14 23:39:27 UTC (rev 222060)
@@ -1,3 +1,15 @@
+2017-09-14 Saam Barati <sbar...@apple.com>
+
+ It should be valid to exit before each set when doing arity fixup when inlining
+ https://bugs.webkit.org/show_bug.cgi?id=176948
+
+ Reviewed by Keith Miller.
+
+ * stress/arity-fixup-inlining-dont-generate-invalid-use.js: Added.
+ (baz):
+ (bar):
+ (foo):
+
2017-09-14 Yusuke Suzuki <utatane....@gmail.com>
[JSC] Add PrivateSymbolMode::{Include,Exclude} for PropertyNameArray
Added: trunk/JSTests/stress/arity-fixup-inlining-dont-generate-invalid-use.js (0 => 222060)
--- trunk/JSTests/stress/arity-fixup-inlining-dont-generate-invalid-use.js (rev 0)
+++ trunk/JSTests/stress/arity-fixup-inlining-dont-generate-invalid-use.js 2017-09-14 23:39:27 UTC (rev 222060)
@@ -0,0 +1,26 @@
+function baz() { }
+noInline(baz);
+
+function bar(x, y, z) {
+ baz(z);
+ return x + y + 20.2;
+}
+function foo(x, b) {
+ let y = x + 10.54;
+ let z = y;
+ if (b) {
+ y += 1.23;
+ z += 2.199;
+ } else {
+ y += 2.27;
+ z += 2.18;
+ }
+
+ let r = bar(b ? y : z, !b ? y : z);
+
+ return r;
+}
+noInline(foo);
+
+for (let i = 0; i < 1000; ++i)
+ foo(i+0.5, !!(i%2));
Modified: trunk/Source/_javascript_Core/ChangeLog (222059 => 222060)
--- trunk/Source/_javascript_Core/ChangeLog 2017-09-14 23:37:32 UTC (rev 222059)
+++ trunk/Source/_javascript_Core/ChangeLog 2017-09-14 23:39:27 UTC (rev 222060)
@@ -1,3 +1,24 @@
+2017-09-14 Saam Barati <sbar...@apple.com>
+
+ It should be valid to exit before each set when doing arity fixup when inlining
+ https://bugs.webkit.org/show_bug.cgi?id=176948
+
+ Reviewed by Keith Miller.
+
+ This patch makes it so that we can exit before each SetLocal when doing arity
+ fixup during inlining. This is OK because if we exit at any of these SetLocals,
+ we will simply exit to the beginning of the call instruction.
+
+ Not doing this led to a bug where FixupPhase would insert a ValueRep of
+ a node before the actual node. This is obviously invalid IR. I've added
+ a new validation rule to catch this malformed IR.
+
+ * dfg/DFGByteCodeParser.cpp:
+ (JSC::DFG::ByteCodeParser::inliningCost):
+ (JSC::DFG::ByteCodeParser::inlineCall):
+ * dfg/DFGValidate.cpp:
+ * runtime/Options.h:
+
2017-09-14 Mark Lam <mark....@apple.com>
AddressSanitizer: stack-buffer-underflow in JSC::Probe::Page::Page
Modified: trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (222059 => 222060)
--- trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2017-09-14 23:37:32 UTC (rev 222059)
+++ trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2017-09-14 23:39:27 UTC (rev 222060)
@@ -1456,7 +1456,6 @@
return UINT_MAX;
}
-
if (!Options::useArityFixupInlining()) {
if (codeBlock->numParameters() > argumentCountIncludingThis) {
if (DFGByteCodeParserInternal::verbose)
@@ -1582,8 +1581,12 @@
if (arityFixupCount) {
Node* undefined = addToGraph(JSConstant, OpInfo(m_constantUndefined));
auto fill = [&] (VirtualRegister reg, Node* value) {
- Node* result = set(reg, value, ImmediateNakedSet);
- result->variableAccessData()->mergeShouldNeverUnbox(true); // We cannot exit after starting arity fixup.
+ // It's valid to exit here since we'll exit to the top of the
+ // call and re-setup the arguments.
+ m_exitOK = true;
+ addToGraph(ExitOK);
+
+ set(reg, value, ImmediateNakedSet);
};
// The stack needs to be aligned due to ABIs. Thus, we have a hole if the count of arguments is not aligned.
Modified: trunk/Source/_javascript_Core/dfg/DFGValidate.cpp (222059 => 222060)
--- trunk/Source/_javascript_Core/dfg/DFGValidate.cpp 2017-09-14 23:37:32 UTC (rev 222059)
+++ trunk/Source/_javascript_Core/dfg/DFGValidate.cpp 2017-09-14 23:39:27 UTC (rev 222060)
@@ -425,6 +425,18 @@
VALIDATE((node, edge), m_acceptableNodes.contains(edge.node()));
}
}
+
+ {
+ HashSet<Node*> seenNodes;
+ for (size_t i = 0; i < block->size(); ++i) {
+ Node* node = block->at(i);
+ m_graph.doToChildren(node, [&] (const Edge& edge) {
+ Node* child = edge.node();
+ VALIDATE((node), block->isInPhis(child) || seenNodes.contains(child));
+ });
+ seenNodes.add(node);
+ }
+ }
for (size_t i = 0; i < block->phis.size(); ++i) {
Node* node = block->phis[i];
Modified: trunk/Source/_javascript_Core/runtime/Options.h (222059 => 222060)
--- trunk/Source/_javascript_Core/runtime/Options.h 2017-09-14 23:37:32 UTC (rev 222059)
+++ trunk/Source/_javascript_Core/runtime/Options.h 2017-09-14 23:39:27 UTC (rev 222060)
@@ -257,7 +257,7 @@
v(bool, useMovHintRemoval, true, Normal, nullptr) \
v(bool, usePutStackSinking, true, Normal, nullptr) \
v(bool, useObjectAllocationSinking, true, Normal, nullptr) \
- v(bool, useArityFixupInlining, false, Normal, nullptr) \
+ v(bool, useArityFixupInlining, true, Normal, nullptr) \
v(bool, logExecutableAllocation, false, Normal, nullptr) \
\
v(bool, useConcurrentJIT, true, Normal, "allows the DFG / FTL compilation in threads other than the executing JS thread") \
