It should be valid to exit before each set when doing arity fixup when inlining
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 Sep 2017 23:39:27 +0000 (23:39 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 Sep 2017 23:39:27 +0000 (23:39 +0000)
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/JavaScriptCore:

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:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@222060 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/arity-fixup-inlining-dont-generate-invalid-use.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Source/JavaScriptCore/dfg/DFGValidate.cpp
Source/JavaScriptCore/runtime/Options.h

index 6c30641..41e5a41 100644 (file)
@@ -1,3 +1,15 @@
+2017-09-14  Saam Barati  <sbarati@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.tea@gmail.com>
 
         [JSC] Add PrivateSymbolMode::{Include,Exclude} for PropertyNameArray
diff --git a/JSTests/stress/arity-fixup-inlining-dont-generate-invalid-use.js b/JSTests/stress/arity-fixup-inlining-dont-generate-invalid-use.js
new file mode 100644 (file)
index 0000000..17f2311
--- /dev/null
@@ -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));
index de07d19..bf7ea2c 100644 (file)
@@ -1,3 +1,24 @@
+2017-09-14  Saam Barati  <sbarati@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.lam@apple.com>
 
         AddressSanitizer: stack-buffer-underflow in JSC::Probe::Page::Page
index 963949e..9cf430a 100644 (file)
@@ -1456,7 +1456,6 @@ unsigned ByteCodeParser::inliningCost(CallVariant callee, int argumentCountInclu
         return UINT_MAX;
     }
 
-
     if (!Options::useArityFixupInlining()) {
         if (codeBlock->numParameters() > argumentCountIncludingThis) {
             if (DFGByteCodeParserInternal::verbose)
@@ -1582,8 +1581,12 @@ void ByteCodeParser::inlineCall(Node* callTargetNode, int resultOperand, CallVar
     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.
index e147e48..c9ab2b4 100644 (file)
@@ -425,6 +425,18 @@ private:
                     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];
index c928a51..3cec552 100644 (file)
@@ -257,7 +257,7 @@ typedef const char* optionString;
     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") \