Bug 138252 - Add scope operand to op_push_with_scope, op_push_name_scope and op_pop_scope
Summary: Add scope operand to op_push_with_scope, op_push_name_scope and op_pop_scope
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
Depends on:
Blocks: 136724 138254
  Show dependency treegraph
Reported: 2014-10-31 11:05 PDT by Michael Saboff
Modified: 2014-10-31 14:27 PDT (History)
2 users (show)

See Also:

Patch (18.78 KB, patch)
2014-10-31 11:23 PDT, Michael Saboff
ggaren: review-
Details | Formatted Diff | Diff
Patch with suggested updates (20.31 KB, patch)
2014-10-31 12:46 PDT, Michael Saboff
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2014-10-31 11:05:42 PDT
This is part of the effort to remove the scope chain from the call frame header (https://bugs.webkit.org/show_bug.cgi?id=136724).

This bug is to add a source scope register index to the op_push_with_scope, op_push_name_scope and op_pop_scope byte codes.  The new operand will be filled in but not used.
Comment 1 Michael Saboff 2014-10-31 11:23:54 PDT
Created attachment 240746 [details]
Comment 2 Geoffrey Garen 2014-10-31 11:37:44 PDT
Comment on attachment 240746 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=240746&action=review

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2199
> +void BytecodeGenerator::emitComplexPopScopes(RegisterID* scopeReg, ControlFlowContext* topScope, ControlFlowContext* bottomScope)

Let's call this "scope", or, if we need to disambiguate from something else named "scope", then "scopeRegister".

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2314
> +void BytecodeGenerator::emitPopScopes(RegisterID* scopeReg, int targetScopeDepth)


> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:543
> +        void emitPopScope(RegisterID* scopeReg);

This should be "srcDst" to match other functions' naming.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2253
> +    RegisterID scopeDest(JSStack::ScopeChain);

Let's make the scope register a data member in the BytecodeGenerator instead of a one-off allocation at the call site. It's not really valid to allocate bytecode registers in this way.
Comment 3 Michael Saboff 2014-10-31 12:46:01 PDT
Created attachment 240749 [details]
Patch with suggested updates
Comment 4 Geoffrey Garen 2014-10-31 13:29:03 PDT
Comment on attachment 240749 [details]
Patch with suggested updates

Comment 5 Michael Saboff 2014-10-31 14:27:03 PDT
Committed r175426: <http://trac.webkit.org/changeset/175426>