[hyjorc1]

d7468b4

[JSC] Add a resolve_and_get_from_scope bytecode opcode
https://bugs.webkit.org/show_bug.cgi?id=237836

Reviewed by NOBODY (OOPS!).

In generated bytecodes, resolve_scope followed by get_from_scope is common.
This patch introduces a new bytecode opcode resolve_and_get_from_scope by
replacing the resolve_scope and get_from_scope pair in the generated instruction stream.

Bytecodes Before:
...
[  28] resolve_scope      dst:loc10, scope:loc4, var:1, resolveType:GlobalProperty, localScopeDepth:0
[  35] get_from_scope     dst:loc7, scope:loc10, var:1, getPutInfo:2048<...>, localScopeDepth:0, offset:0
...

Bytecodes After:
...
[  28] resolve_and_get_from_scope dst:loc7, scope:loc4, resolvedScope:loc10, var:1, resolveType:GlobalProperty, getPutInfo:2048<...>, localScopeDepth:0, offset:0
...

Reviewed by NOBODY (OOPS!).

* Source/JavaScriptCore/bytecode/BytecodeList.rb:
* Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:
(JSC::tmpLivenessForCheckpoint):
* Source/JavaScriptCore/bytecode/BytecodeOperandsForCheckpoint.h:
(JSC::valueProfileForImpl):
(JSC::valueProfileFor):
* Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp:
(JSC::computeUsesForBytecodeIndexImpl):
(JSC::computeDefsForBytecodeIndexImpl):
* Source/JavaScriptCore/bytecode/CodeBlock.cpp:
(JSC::CodeBlock::finishCreation):
(JSC::CodeBlock::finalizeLLIntInlineCaches):
(JSC::CodeBlock::notifyLexicalBindingUpdate):
(JSC::CodeBlock::tryGetValueProfileForBytecodeIndex):
* Source/JavaScriptCore/bytecode/CodeBlockInlines.h:
(JSC::CodeBlock::forEachValueProfile):
* Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:
(JSC::UnlinkedCodeBlock::allocateSharedProfiles):
* Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::initializeDefaultParameterValuesAndSetupFunctionScopeStack):
(JSC::BytecodeGenerator::hoistSloppyModeFunctionIfNecessary):
(JSC::BytecodeGenerator::emitResolveScopeWithVarKindScope):
(JSC::BytecodeGenerator::emitResolveScopeHelper):
(JSC::BytecodeGenerator::emitResolveScope):
(JSC::BytecodeGenerator::emitGetFromScopeWithVarKindStack):
(JSC::BytecodeGenerator::emitGetFromScopeWithVarKindDirectArgument):
(JSC::BytecodeGenerator::emitGetFromScopeHelper):
(JSC::BytecodeGenerator::emitGetFromScope):
(JSC::BytecodeGenerator::emitResolveAndGetFromScope):
(JSC::BytecodeGenerator::emitLoadThisFromArrowFunctionLexicalEnvironment):
(JSC::BytecodeGenerator::emitGetGlobalPrivate): Deleted.
* Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:
* Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:
(JSC::ResolveNode::emitBytecode):
(JSC::BaseDotNode::emitGetPropertyValue):
(JSC::BaseDotNode::emitPutProperty):
(JSC::PostfixNode::emitDot):
(JSC::PrefixNode::emitDot):
(JSC::EmptyVarExpression::emitBytecode):
(JSC::DefineFieldNode::emitBytecode):
* Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::needsDynamicLookup):
(JSC::DFG::ByteCodeParser::parseBlock):
* Source/JavaScriptCore/dfg/DFGCapabilities.cpp:
(JSC::DFG::capabilityLevel):
* Source/JavaScriptCore/jit/JIT.cpp:
(JSC::JIT::privateCompileMainPass):
* Source/JavaScriptCore/jit/JIT.h:
* Source/JavaScriptCore/jit/JITOperations.cpp:
(JSC::operationResolveScopeHelper):
(JSC::JSC_DEFINE_JIT_OPERATION):
(JSC::operationGetFromScopeHelper):
* Source/JavaScriptCore/jit/JITOperations.h:
* Source/JavaScriptCore/jit/JITPropertyAccess.cpp:
(JSC::JIT::emit_op_resolve_scope_helper):
(JSC::JIT::emit_op_resolve_scope):
(JSC::JIT::generateOpResolveScopeThunkHelper):
(JSC::JIT::generateOpResolveScopeThunk):
(JSC::JIT::slow_op_resolve_scopeGenerator_helper):
(JSC::JIT::slow_op_resolve_scopeGenerator):
(JSC::JIT::emit_op_get_from_scope_helper):
(JSC::JIT::emit_op_get_from_scope):
(JSC::JIT::generateOpGetFromScopeThunk):
(JSC::JIT::generateOpGetFromScopeThunkHelper):
(JSC::JIT::slow_op_get_from_scopeGenerator):
(JSC::JIT::slow_op_get_from_scopeGenerator_helper):
(JSC::JIT::emit_op_resolve_and_get_from_scope):
(JSC::JIT::generateOpRGSResolveScopeThunk):
(JSC::JIT::generateOpRGSGetFromScopeThunk):
(JSC::JIT::slow_op_rgs_resolve_scopeGenerator):
(JSC::JIT::slow_op_rgs_get_from_scopeGenerator):
* Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:
(JSC::LLInt::scopeGetPropertyHelper):
(JSC::LLInt::llintSlowPathGetFromScopeHelper):
(JSC::LLInt::LLINT_SLOW_PATH_DECL):
(JSC::LLInt::handleResolveAndGetFromScopeCheckpoint):
(JSC::LLInt::llint_slow_path_checkpoint_osr_exit):
* Source/JavaScriptCore/llint/LLIntSlowPaths.h:
* Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:
* Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:
* Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:
(JSC::commonSlowPathResolveScopeHelper):
(JSC::JSC_DEFINE_COMMON_SLOW_PATH):
* Source/JavaScriptCore/runtime/CommonSlowPaths.h:
* Source/JavaScriptCore/runtime/CommonSlowPathsInlines.h:
(JSC::CommonSlowPaths::tryCacheGetFromScopeGlobal):

[saambarati]

I'd try to work on sharing a bit more code between the implementations, and getting this patch into a state where it can be reviewed a bit more easily. Right now, it contains a lot of duplicate code.

[justinmichaud]

I have a few questions about the code, maybe we can brainstorm later with Yusuke?

[justinmichaud]

Do we know why this is here?

[hyjorc1]

This is extracted from the resolve scope switch case.

[justinmichaud]

Maybe a switch would be better, since it would be easier to grep for? Or perhaps the dst can be passed in as a parameter?

[hyjorc1]

Updated

[justinmichaud]

This seems sub-optimal? Maybe @Constellation has some thoughts? Do iterators have to do something similar?

[justinmichaud]

Since we only call this once, can this just be a bool?

[hyjorc1]

Updated

[justinmichaud]

Why do we set two things here?

[hyjorc1]

tmpResolvedScope is used to store the resolved scope for OSR exit.

[geoffreygaren]

What's the motivation here? Memory use? Startup time? Revealing a new optimization to later compilers?

[justinmichaud]

What's the motivation here? Memory use? Startup time? Revealing a new optimization to later compilers?

No-jit memory usage in particular

[hyjorc1]

What's the motivation here? Memory use? Startup time? Revealing a new optimization to later compilers?

Initially, we believe the new instruction could help us to reduce the total bytecode size in order to reduce memory usage, specifically for RAMification.

[saambarati]

Why are we padding dst as the resolved scope here? That looks quite wrong.

[hyjorc1]

I think the logic is correct. But, the function name emitResolveAndGetFromScope is misleading and it should be tryEmitResolveAndGetFromScope. If you check the implementation of this function:

RegisterID* BytecodeGenerator::emitResolveAndGetFromScope(RegisterID* dst, RegisterID* resolvedScope, const Variable& variable, ResolveMode resolveMode, bool isPrivate)
{
    switch (variable.offset().kind()) {
    case VarKind::Stack: {
        ASSERT(!isPrivate);
        UNUSED_VARIABLE(isPrivate);
        return emitGetFromScopeWithVarKindStack(dst, variable);
    }
    case VarKind::DirectArgument:
        return emitGetFromScopeWithVarKindDirectArgument(dst, argumentsRegister(), variable);
    case VarKind::Scope:
        return emitGetFromScopeHelper(dst, emitResolveScopeWithVarKindScope(variable), variable, resolveMode);
    case VarKind::Invalid: {
#if USE(JSVALUE64)
        if (!variable.offset().isScope()) {
            OpResolveAndGetFromScope::emit(
                this,
                kill(dst),
                scopeRegister(),
                addConstant(variable.ident()),
                GetPutInfo(resolveMode, resolveType(), InitializationMode::NotInitialization, ecmaMode()),
                localScopeDepth(),
                variable.offset().isScope() ? variable.offset().scopeOffset().offset() : 0);
            return dst;
        }
#endif
        return emitGetFromScopeHelper(dst, emitResolveScopeHelper(resolvedScope, variable), variable, resolveMode);
    }
    }

    RELEASE_ASSERT_NOT_REACHED();
    return nullptr;
}

which still need to handle the non-ResolveAndGetFromScope cases. Here, the dst is passed to the function as the resolvedScope for clause:

return emitGetFromScopeHelper(dst, emitResolveScopeHelper(resolvedScope, variable), variable, resolveMode);

[hyjorc1]

I believe the similar situation happens in both emitResolveScope and emitGetFromScope. And they should be tryEmitResolveScope and tryEmitGetFromScope instead.

[saambarati]

They should not be prefixed with try. Whether or not we actually emit the opcodes, we're logically resolving and getting from a scope. I don't see why we need to pass in the dst as the result of the resolve_scope. If we need the scope for later operations, we shouldn't be emitting resolve_and_get_from_scope combo opcode.

[saambarati]

I don't see a reason why we need a resolvedScope parameter here. The one call site that's passing something other than nullptr is looks wrong to me. I think it's actually better if the contract of this API is that we don't define a scope, so we can remove the operand from the bytecode

[hyjorc1]

Yes, we need resolvedScope for handling non-ResolveAndGetFromScope cases introduced in emitResolveAndGetFromScope.

[saambarati]

nit: this can be an else if

[hyjorc1]

Updated

[saambarati]

This doesn't look right. It's not putting the result in dst

[saambarati]

Also, this would be a compile error once we get rid of that scope field we no longer need from the bytecode

[hyjorc1]

This doesn't look right. It's not putting the result in dst

I think this is correct. Here, the dst is passed to the helper function resolveScopeHelper , which is extracted from case op_resolve_scope: and it is for both op_resolve_scope and op_resolve_and_get_from_scope. Each case will pass their own dst, which is the a destination register or a Tmp variable for op_resolve_scope or op_resolve_and_get_from_scope respectively.

Also, this would be a compile error once we get rid of that scope field we no longer need from the bytecode

I don't think we can remove the scope operand or field.

[saambarati]

Can this be a function instead of a macro?

[hyjorc1]

The passed thunk function is a template function which cannot be passed as a function pointer. If we need to use function, then we have to create two functions for OpGetFromScope and OpResolveScope each.

[saambarati]

ditto, can this be a function instead of a macro?

[hyjorc1]

ditto, are two functions better than one macro?

[saambarati]

nit: Let's not abbreviate this name this way. Write out the full thing

[hyjorc1]

Updated

[saambarati]

We should refactor this in such a way we don't need to write out the intermediate resolve_scope result to the stack. There is no need to do that, so we should avoid it

[hyjorc1]

The current implementation stores the intermediate resolve_scope result to the m_dst of op_resovle_and_get_from_scope. So, it doesn't need extra space in the stack to keep it. Are you proposing that we should store it to an GPR instead? Could that perform better performance in read and write?

[saambarati]

This can just be #include "CallFrame.h"

[hyjorc1]

Updated

[saambarati]

nit: same thing about writing out the full name

[hyjorc1]

Updated