[Baseline] Remove a hack for DCE removal of NewFunction
authorutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 25 May 2018 04:29:13 +0000 (04:29 +0000)
committerutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 25 May 2018 04:29:13 +0000 (04:29 +0000)
https://bugs.webkit.org/show_bug.cgi?id=185945

Reviewed by Saam Barati.

This `undefined` check in baseline is originally introduced in r177871. The problem was,
when NewFunction is removed in DFG DCE, its referencing scope DFG node  is also removed.
While op_new_func_xxx want to have scope for function creation, DFG OSR exit cannot
retrieve this into the stack since the scope is not referenced from anywhere.

In r177871, we fixed this by accepting `undefined` scope in the baseline op_new_func_xxx
implementation. But rather than that, just emitting `Phantom` for this scope is clean
and consistent to the other DFG nodes like GetClosureVar.

This patch emits Phantom instead, and removes unnecessary `undefined` check in baseline.
While we emit Phantom, it is not testable since NewFunction is guarded by MovHint which
is not removed in DFG. And in FTL, NewFunction will be converted to PhantomNewFunction
if it is not referenced. And scope node is kept by PutHint. But emitting Phantom is nice
since it conservatively guards the scope, and it does not introduce any additional overhead
compared to the current status.

* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::parseBlock):
* jit/JITOpcodes.cpp:
(JSC::JIT::emitNewFuncExprCommon):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Source/JavaScriptCore/jit/JITOpcodes.cpp

index 3e27107..d25c7f6 100644 (file)
@@ -1,3 +1,31 @@
+2018-05-24  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [Baseline] Remove a hack for DCE removal of NewFunction
+        https://bugs.webkit.org/show_bug.cgi?id=185945
+
+        Reviewed by Saam Barati.
+
+        This `undefined` check in baseline is originally introduced in r177871. The problem was,
+        when NewFunction is removed in DFG DCE, its referencing scope DFG node  is also removed.
+        While op_new_func_xxx want to have scope for function creation, DFG OSR exit cannot
+        retrieve this into the stack since the scope is not referenced from anywhere.
+
+        In r177871, we fixed this by accepting `undefined` scope in the baseline op_new_func_xxx
+        implementation. But rather than that, just emitting `Phantom` for this scope is clean
+        and consistent to the other DFG nodes like GetClosureVar.
+
+        This patch emits Phantom instead, and removes unnecessary `undefined` check in baseline.
+        While we emit Phantom, it is not testable since NewFunction is guarded by MovHint which
+        is not removed in DFG. And in FTL, NewFunction will be converted to PhantomNewFunction
+        if it is not referenced. And scope node is kept by PutHint. But emitting Phantom is nice
+        since it conservatively guards the scope, and it does not introduce any additional overhead
+        compared to the current status.
+
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::parseBlock):
+        * jit/JITOpcodes.cpp:
+        (JSC::JIT::emitNewFuncExprCommon):
+
 2018-05-23  Keith Miller  <keith_miller@apple.com>
 
         Expose $vm if window.internals is exposed
index 0784441..fad17fe 100644 (file)
@@ -6327,7 +6327,16 @@ void ByteCodeParser::parseBlock(unsigned limit)
             default:
                 op = NewFunction;
             }
-            set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(op, OpInfo(frozen), get(VirtualRegister(currentInstruction[2].u.operand))));
+            Node* scope = get(VirtualRegister(currentInstruction[2].u.operand));
+            set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(op, OpInfo(frozen), scope));
+            // Ideally we wouldn't have to do this Phantom. But:
+            //
+            // For the constant case: we must do it because otherwise we would have no way of knowing
+            // that the scope is live at OSR here.
+            //
+            // For the non-constant case: NewFunction could be DCE'd, but baseline's implementation
+            // won't be able to handle an Undefined scope.
+            addToGraph(Phantom, scope);
             static_assert(OPCODE_LENGTH(op_new_func) == OPCODE_LENGTH(op_new_generator_func), "The length of op_new_func should be equal to one of op_new_generator_func");
             static_assert(OPCODE_LENGTH(op_new_func) == OPCODE_LENGTH(op_new_async_func), "The length of op_new_func should be equal to one of op_new_async_func");
             static_assert(OPCODE_LENGTH(op_new_func) == OPCODE_LENGTH(op_new_async_generator_func), "The length of op_new_func should be equal to one of op_new_async_generator_func");
@@ -6354,8 +6363,16 @@ void ByteCodeParser::parseBlock(unsigned limit)
             default:
                 op = NewFunction;
             }
-            set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(op, OpInfo(frozen), get(VirtualRegister(currentInstruction[2].u.operand))));
-    
+            Node* scope = get(VirtualRegister(currentInstruction[2].u.operand));
+            set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(op, OpInfo(frozen), scope));
+            // Ideally we wouldn't have to do this Phantom. But:
+            //
+            // For the constant case: we must do it because otherwise we would have no way of knowing
+            // that the scope is live at OSR here.
+            //
+            // For the non-constant case: NewFunction could be DCE'd, but baseline's implementation
+            // won't be able to handle an Undefined scope.
+            addToGraph(Phantom, scope);
             static_assert(OPCODE_LENGTH(op_new_func_exp) == OPCODE_LENGTH(op_new_generator_func_exp), "The length of op_new_func_exp should be equal to one of op_new_generator_func_exp");
             static_assert(OPCODE_LENGTH(op_new_func_exp) == OPCODE_LENGTH(op_new_async_func_exp), "The length of op_new_func_exp should be equal to one of op_new_async_func_exp");
             static_assert(OPCODE_LENGTH(op_new_func_exp) == OPCODE_LENGTH(op_new_async_generator_func_exp), "The length of op_new_func_exp should be equal to one of op_new_async_func_exp");
index be8b15b..38621d8 100644 (file)
@@ -1035,20 +1035,13 @@ void JIT::emit_op_new_async_func(Instruction* currentInstruction)
     
 void JIT::emitNewFuncExprCommon(Instruction* currentInstruction)
 {
-    Jump notUndefinedScope;
     int dst = currentInstruction[1].u.operand;
 #if USE(JSVALUE64)
     emitGetVirtualRegister(currentInstruction[2].u.operand, regT0);
-    notUndefinedScope = branchIfNotUndefined(regT0);
 #else
     emitLoadPayload(currentInstruction[2].u.operand, regT0);
-    notUndefinedScope = branch32(NotEqual, tagFor(currentInstruction[2].u.operand), TrustedImm32(JSValue::UndefinedTag));
 #endif
-    storeTrustedValue(jsUndefined(), addressFor(dst));
 
-    Jump done = jump();
-    notUndefinedScope.link(this);
-        
     FunctionExecutable* function = m_codeBlock->functionExpr(currentInstruction[3].u.operand);
     OpcodeID opcodeID = Interpreter::getOpcodeID(currentInstruction->u.opcode);
 
@@ -1062,8 +1055,6 @@ void JIT::emitNewFuncExprCommon(Instruction* currentInstruction)
         ASSERT(opcodeID == op_new_async_generator_func_exp);
         callOperation(operationNewAsyncGeneratorFunction, dst, regT0, function);
     }
-
-    done.link(this);
 }
 
 void JIT::emit_op_new_func_exp(Instruction* currentInstruction)