Bug 138707 - Add scope operand to op_new_func* byte codes
Summary: Add scope operand to op_new_func* byte codes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 312.x
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks: 136724
  Show dependency treegraph
 
Reported: 2014-11-13 14:15 PST by Michael Saboff
Modified: 2014-11-13 17:07 PST (History)
0 users

See Also:


Attachments
Patch (26.58 KB, patch)
2014-11-13 14:48 PST, Michael Saboff
mark.lam: 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-11-13 14:15:11 PST
These byte codes currently use implicit exec->scope() to get there scope.  This needs to be changed to use a passed in scope.
Comment 1 Michael Saboff 2014-11-13 14:48:54 PST
Created attachment 241505 [details]
Patch
Comment 2 Mark Lam 2014-11-13 15:28:00 PST
Comment on attachment 241505 [details]
Patch

r=me
Comment 3 Geoffrey Garen 2014-11-13 15:35:41 PST
Comment on attachment 241505 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4519
> +        SpeculateCellOperand scope(this, node->child2());

You don't want SpeculateCellOperand here; that will emit a branch to check that the scope is a cell, even though we are certain that it is a cell.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4549
> +        SpeculateCellOperand scope(this, node->child2());

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4162
> +    SpeculateCellOperand scope(this, node->child1());

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4174
> +    SpeculateCellOperand scope(this, node->child1());

Ditto.
Comment 4 Michael Saboff 2014-11-13 16:55:02 PST
(In reply to comment #3)
> Comment on attachment 241505 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241505&action=review
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:4519
> > +        SpeculateCellOperand scope(this, node->child2());
> 
> You don't want SpeculateCellOperand here; that will emit a branch to check
> that the scope is a cell, even though we are certain that it is a cell.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4549
> > +        SpeculateCellOperand scope(this, node->child2());
> 
> Ditto.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4162
> > +    SpeculateCellOperand scope(this, node->child1());
> 
> Ditto.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4174
> > +    SpeculateCellOperand scope(this, node->child1());
> 
> Ditto.

The way I read fillSpeculateCell(), we won't issue the check for DataFormatCell or DataFormatJSCell:

    case DataFormatCell:
    case DataFormatJSCell: {
        GPRReg gpr = info.gpr();
        m_gprs.lock(gpr);
        if (!ASSERT_DISABLED) {
            MacroAssembler::Jump checkCell = branchIsCell(JSValueRegs(gpr));
            m_jit.abortWithReason(DFGIsNotCell);
            checkCell.link(&m_jit);
        }
        return gpr;
    }

I'm just following the existing pattern for other accesses to the scope chain.

    case SkipScope: {
        SpeculateCellOperand scope(this, node->child1());
        GPRTemporary result(this, Reuse, scope);
        m_jit.loadPtr(JITCompiler::Address(scope.gpr(), JSScope::offsetOfNext()), result.gpr());
        cellResult(result.gpr(), node);
        break;
    }

    case GetClosureRegisters: {
 ...        
        SpeculateCellOperand scope(this, node->child1());
        GPRTemporary result(this);
        GPRReg scopeGPR = scope.gpr();
        GPRReg resultGPR = result.gpr();

The current code that uses the call frame ScopeChain register slot will either have the VirtualRegister as a cell or a constant.  For non-inlined frames the child is from a GetMyScope node, which returns a cellResult.  For inlined frames, child is from a JSConstant of the JSScope* value.  Which won't create a check.

When this work is done, op_get_scope will turn into a GetScope node which also has a cellResult.
Comment 5 Michael Saboff 2014-11-13 17:07:44 PST
Committed r176109: <http://trac.webkit.org/changeset/176109>