Title: [232182] trunk/Source/_javascript_Core
- Revision
- 232182
- Author
- [email protected]
- Date
- 2018-05-24 21:29:13 -0700 (Thu, 24 May 2018)
Log Message
[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):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (232181 => 232182)
--- trunk/Source/_javascript_Core/ChangeLog 2018-05-25 03:21:28 UTC (rev 232181)
+++ trunk/Source/_javascript_Core/ChangeLog 2018-05-25 04:29:13 UTC (rev 232182)
@@ -1,3 +1,31 @@
+2018-05-24 Yusuke Suzuki <[email protected]>
+
+ [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 <[email protected]>
Expose $vm if window.internals is exposed
Modified: trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (232181 => 232182)
--- trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2018-05-25 03:21:28 UTC (rev 232181)
+++ trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2018-05-25 04:29:13 UTC (rev 232182)
@@ -6327,7 +6327,16 @@
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 @@
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");
Modified: trunk/Source/_javascript_Core/jit/JITOpcodes.cpp (232181 => 232182)
--- trunk/Source/_javascript_Core/jit/JITOpcodes.cpp 2018-05-25 03:21:28 UTC (rev 232181)
+++ trunk/Source/_javascript_Core/jit/JITOpcodes.cpp 2018-05-25 04:29:13 UTC (rev 232182)
@@ -1035,20 +1035,13 @@
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 @@
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)
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
