Attachment 341244 Details for Bug 185945 – Patch

[patch] Patch

bug-185945-20180525121718.patch (text/plain), 6.41 KB, created by Yusuke Suzuki on 2018-05-24 20:17:19 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2018-05-24 20:17:19 PDT

Size: 6.41 KB

patch

obsolete

>Subversion Revision: 232180
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 3e27107cfe4ea3c1b6381a9e1125c6d179eb91b6..e3c378a5595e948c015d59ca729f61d93c0d1114 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        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
>diff --git a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>index 078444138751b6a99621076c986035ac7ba81da4..96f6f11c1e6fc63a990fd30e9c1074139bc29cdc 100644
>--- a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>@@ -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));
>+            // 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);
>+            set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(op, OpInfo(frozen), 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,7 +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));
>+            // 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);
>+            set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(op, OpInfo(frozen), 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");
>diff --git a/Source/JavaScriptCore/jit/JITOpcodes.cpp b/Source/JavaScriptCore/jit/JITOpcodes.cpp
>index be8b15b65a0ee3bf2e45ed356b2210615291b691..38621d886e70c4158ecd7a8eaa9f6dc1105cb829 100644
>--- a/Source/JavaScriptCore/jit/JITOpcodes.cpp
>+++ b/Source/JavaScriptCore/jit/JITOpcodes.cpp
>@@ -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)

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 185945: 341189 | 341244