Arity fixup during inlining should do a 2 phase commit so it properly recovers the...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Sep 2017 23:27:56 +0000 (23:27 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Sep 2017 23:27:56 +0000 (23:27 +0000)
https://bugs.webkit.org/show_bug.cgi?id=176981

Reviewed by Yusuke Suzuki.

JSTests:

* stress/exit-during-inlined-arity-fixup-recover-proper-frame.js: Added.
(assert):
(verify):
(func):
(const.bar.createBuiltin):

Source/JavaScriptCore:

This patch makes inline arity fixup happen in two phases:
1. We get all the values we need and MovHint them to the expected locals.
2. We SetLocal them inside the callee's CodeOrigin. This way, if we exit, the callee's
   frame is already set up. If any SetLocal exits, we have a valid exit state.
   This is required because if we didn't do this in two phases, we may exit in
   the middle of arity fixup from the caller's CodeOrigin. This is unsound because if
   we did the SetLocals in the caller's frame, the memcpy may clobber needed parts
   of the frame right before exiting. For example, consider if we need to pad two args:
   [arg3][arg2][arg1][arg0]
   [fix ][fix ][arg3][arg2][arg1][arg0]
   We memcpy starting from arg0 in the direction of arg3. If we were to exit at a type check
   for arg3's SetLocal in the caller's CodeOrigin, we'd exit with a frame like so:
   [arg3][arg2][arg1][arg2][arg1][arg0]
   And the caller would then just end up thinking its argument are:
   [arg3][arg2][arg1][arg2]
   which is incorrect.

This patch also fixes a couple of bugs in IdentitiyWithProfile:
1. The bytecode generator for this bytecode intrinsic was written incorrectly.
   It needed to store the result of evaluating its argument in a temporary that
   it creates. Otherwise, it might try to simply overwrite a constant
   or a register that it didn't own.
2. We weren't eliminating this node in CSE inside the DFG.

* bytecompiler/NodesCodegen.cpp:
(JSC::BytecodeIntrinsicNode::emit_intrinsic_idWithProfile):
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::inlineCall):
* dfg/DFGCSEPhase.cpp:

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

JSTests/ChangeLog
JSTests/stress/exit-during-inlined-arity-fixup-recover-proper-frame.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Source/JavaScriptCore/dfg/DFGCSEPhase.cpp

index 41e5a41..b74b1e4 100644 (file)
@@ -1,3 +1,16 @@
+2017-09-15  Saam Barati  <sbarati@apple.com>
+
+        Arity fixup during inlining should do a 2 phase commit so it properly recovers the frame in case of exit
+        https://bugs.webkit.org/show_bug.cgi?id=176981
+
+        Reviewed by Yusuke Suzuki.
+
+        * stress/exit-during-inlined-arity-fixup-recover-proper-frame.js: Added.
+        (assert):
+        (verify):
+        (func):
+        (const.bar.createBuiltin):
+
 2017-09-14  Saam Barati  <sbarati@apple.com>
 
         It should be valid to exit before each set when doing arity fixup when inlining
diff --git a/JSTests/stress/exit-during-inlined-arity-fixup-recover-proper-frame.js b/JSTests/stress/exit-during-inlined-arity-fixup-recover-proper-frame.js
new file mode 100644 (file)
index 0000000..11761b9
--- /dev/null
@@ -0,0 +1,31 @@
+let i;
+function verify(a, b, c, d, e, f) {
+    function assert(b, m) {
+        if (!b)
+            throw new Error(m);
+    }
+    assert(a === i);
+    assert(b === i+1);
+    assert(c === i+2);
+    assert(d === null);
+    assert(e === undefined);
+    assert(f === undefined);
+}
+noInline(verify);
+
+function func(a, b, c, d, e, f)
+{
+    verify(a, b, c, d, e, f);
+    return !!(a%2) ? a + b + c + d : a + b + c + d;
+}
+
+const bar = createBuiltin(`(function (f, a, b, c, d) {
+    let y = @idWithProfile(null, "SpecInt32Only");
+    return f(a, b, c, y);
+})`);
+
+noInline(bar);
+
+for (i = 0; i < 1000; ++i) {
+    bar(func, i, i+1, i+2, i+3);
+}
index d5d111d..e7ac0b4 100644 (file)
@@ -1,3 +1,41 @@
+2017-09-15  Saam Barati  <sbarati@apple.com>
+
+        Arity fixup during inlining should do a 2 phase commit so it properly recovers the frame in case of exit
+        https://bugs.webkit.org/show_bug.cgi?id=176981
+
+        Reviewed by Yusuke Suzuki.
+
+        This patch makes inline arity fixup happen in two phases:
+        1. We get all the values we need and MovHint them to the expected locals.
+        2. We SetLocal them inside the callee's CodeOrigin. This way, if we exit, the callee's
+           frame is already set up. If any SetLocal exits, we have a valid exit state.
+           This is required because if we didn't do this in two phases, we may exit in
+           the middle of arity fixup from the caller's CodeOrigin. This is unsound because if
+           we did the SetLocals in the caller's frame, the memcpy may clobber needed parts
+           of the frame right before exiting. For example, consider if we need to pad two args:
+           [arg3][arg2][arg1][arg0]
+           [fix ][fix ][arg3][arg2][arg1][arg0]
+           We memcpy starting from arg0 in the direction of arg3. If we were to exit at a type check
+           for arg3's SetLocal in the caller's CodeOrigin, we'd exit with a frame like so:
+           [arg3][arg2][arg1][arg2][arg1][arg0]
+           And the caller would then just end up thinking its argument are:
+           [arg3][arg2][arg1][arg2]
+           which is incorrect.
+       
+       
+        This patch also fixes a couple of bugs in IdentitiyWithProfile:
+        1. The bytecode generator for this bytecode intrinsic was written incorrectly.
+           It needed to store the result of evaluating its argument in a temporary that
+           it creates. Otherwise, it might try to simply overwrite a constant
+           or a register that it didn't own.
+        2. We weren't eliminating this node in CSE inside the DFG.
+
+        * bytecompiler/NodesCodegen.cpp:
+        (JSC::BytecodeIntrinsicNode::emit_intrinsic_idWithProfile):
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::inlineCall):
+        * dfg/DFGCSEPhase.cpp:
+
 2017-09-15  JF Bastien  <jfbastien@apple.com>
 
         WTF: use Forward.h when appropriate instead of Vector.h
index 9e0dc7f..0dbfeb4 100644 (file)
@@ -1026,9 +1026,9 @@ RegisterID* BytecodeIntrinsicNode::emit_intrinsic_toString(BytecodeGenerator& ge
 
 RegisterID* BytecodeIntrinsicNode::emit_intrinsic_idWithProfile(BytecodeGenerator& generator, RegisterID* dst)
 {
-
     ArgumentListNode* node = m_args->m_listNode;
-    RefPtr<RegisterID> idValue = generator.emitNode(node);
+    RefPtr<RegisterID> idValue = generator.newTemporary();
+    generator.emitNode(idValue.get(), node);
     SpeculatedType speculation = SpecNone;
     while (node->m_next) {
         node = node->m_next;
index 9cf430a..331c406 100644 (file)
@@ -1577,19 +1577,29 @@ void ByteCodeParser::inlineCall(Node* callTargetNode, int resultOperand, CallVar
         calleeVariable = calleeSet->variableAccessData();
         calleeVariable->mergeShouldNeverUnbox(true);
     }
-    
-    if (arityFixupCount) {
-        Node* undefined = addToGraph(JSConstant, OpInfo(m_constantUndefined));
-        auto fill = [&] (VirtualRegister reg, Node* value) {
-            // 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);
-        };
+    Vector<std::pair<Node*, VirtualRegister>> delayedAritySets;
+
+    if (arityFixupCount) {
+        // Note: we do arity fixup in two phases:
+        // 1. We get all the values we need and MovHint them to the expected locals.
+        // 2. We SetLocal them inside the callee's CodeOrigin. This way, if we exit, the callee's
+        //    frame is already set up. If any SetLocal exits, we have a valid exit state.
+        //    This is required because if we didn't do this in two phases, we may exit in
+        //    the middle of arity fixup from the caller's CodeOrigin. This is unsound because if
+        //    we did the SetLocals in the caller's frame, the memcpy may clobber needed parts
+        //    of the frame right before exiting. For example, consider if we need to pad two args:
+        //    [arg3][arg2][arg1][arg0]
+        //    [fix ][fix ][arg3][arg2][arg1][arg0]
+        //    We memcpy starting from arg0 in the direction of arg3. If we were to exit at a type check
+        //    for arg3's SetLocal in the caller's CodeOrigin, we'd exit with a frame like so:
+        //    [arg3][arg2][arg1][arg2][arg1][arg0]
+        //    And the caller would then just end up thinking its argument are:
+        //    [arg3][arg2][arg1][arg2]
+        //    which is incorrect.
 
-        // The stack needs to be aligned due to ABIs. Thus, we have a hole if the count of arguments is not aligned.
+        Node* undefined = addToGraph(JSConstant, OpInfo(m_constantUndefined));
+        // The stack needs to be aligned due to the JS calling convention. Thus, we have a hole if the count of arguments is not aligned.
         // We call this hole "extra slot". Consider the following case, the number of arguments is 2. If this argument
         // count does not fulfill the stack alignment requirement, we already inserted extra slots.
         //
@@ -1603,11 +1613,21 @@ void ByteCodeParser::inlineCall(Node* callTargetNode, int resultOperand, CallVar
         //
         // In such cases, we do not need to move frames.
         if (registerOffsetAfterFixup != registerOffset) {
-            for (int index = 0; index < argumentCountIncludingThis; ++index)
-                fill(virtualRegisterForArgument(index, registerOffsetAfterFixup), get(virtualRegisterForArgument(index, registerOffset)));
+            for (int index = 0; index < argumentCountIncludingThis; ++index) {
+                Node* value = get(virtualRegisterForArgument(index, registerOffset));
+                VirtualRegister argumentToSet = m_inlineStackTop->remapOperand(virtualRegisterForArgument(index, registerOffsetAfterFixup));
+                addToGraph(MovHint, OpInfo(argumentToSet.offset()), value);
+                m_setLocalQueue.append(DelayedSetLocal { currentCodeOrigin(), argumentToSet, value, ImmediateNakedSet });
+            }
         }
-        for (int index = 0; index < arityFixupCount; ++index)
-            fill(virtualRegisterForArgument(argumentCountIncludingThis + index, registerOffsetAfterFixup), undefined);
+        for (int index = 0; index < arityFixupCount; ++index) {
+            VirtualRegister argumentToSet = m_inlineStackTop->remapOperand(virtualRegisterForArgument(argumentCountIncludingThis + index, registerOffsetAfterFixup));
+            addToGraph(MovHint, OpInfo(argumentToSet.offset()), undefined);
+            m_setLocalQueue.append(DelayedSetLocal { currentCodeOrigin(), argumentToSet, undefined, ImmediateNakedSet });
+        }
+
+        // At this point, it's OK to OSR exit because we finished setting up
+        // our callee's frame. We emit an ExitOK below from the callee's CodeOrigin.
     }
 
     InlineStackEntry inlineStackEntry(
@@ -1620,6 +1640,9 @@ void ByteCodeParser::inlineCall(Node* callTargetNode, int resultOperand, CallVar
 
     // At this point, it's again OK to OSR exit.
     m_exitOK = true;
+    addToGraph(ExitOK);
+
+    processSetLocalQueue();
 
     InlineVariableData inlineVariableData;
     inlineVariableData.inlineCallFrame = m_inlineStackTop->m_inlineCallFrame;
index ebd1b8e..eacefb7 100644 (file)
@@ -448,7 +448,7 @@ private:
                 m_node = block->at(nodeIndex);
                 m_graph.performSubstitution(m_node);
             
-                if (m_node->op() == Identity) {
+                if (m_node->op() == Identity || m_node->op() == IdentityWithProfile) {
                     m_node->replaceWith(m_node->child1().node());
                     m_changed = true;
                 } else {
@@ -651,7 +651,7 @@ public:
                 
                 m_graph.performSubstitution(m_node);
                 
-                if (m_node->op() == Identity) {
+                if (m_node->op() == Identity || m_node->op() == IdentityWithProfile) {
                     m_node->replaceWith(m_node->child1().node());
                     m_changed = true;
                 } else