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
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks: 136724 138254
  Show dependency tree / graph
 
Reported: 2014-10-31 11:05 PDT by Michael Saboff
Modified: 2014-10-31 14:27 PDT (History)
2 users (show)

See Also:

Attachments
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
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

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]
Patch
Comment 2 Geoffrey Garen 2014-10-31 11:37:44 PDT 
Comment on attachment 240746 [details]
Patch

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)

Ditto.

> 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

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