Bug 118203 – DFG should support ret_object_or_this

• Status: NEW
• Component: JavaScriptCore

[Mark Hahnenberg]

It's sad that we can't optimize away the result of the create_this bytecode in constructors that create their own objects and return them instead of just modifying "this". We should add support for the constructor_ret to the DFG so that it can do this for us.

[Mark Hahnenberg]

Created attachment 208157 [details]
Patch

[Mark Hahnenberg]

Created attachment 208160 [details]
Patch

[Filip Pizlo]

Comment on attachment 208160 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:941
> +            m_state.setFoundConstants(true);

This looks borked. Why aren't you setting the result of the node?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2823
>          case op_ret:

It looks like this would work better with a helper function, rather than having a bunch of opcode checks.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:3263
> +        cellResult(thisGPR, node);

Yeah if a node returns a value, as this one clearly does, you need to have the abstract interpreter indicate what it returns. Right now you're leaving the result clear which is definitely wrong.

[Mark Hahnenberg]

Created attachment 208210 [details]
Patch

[Filip Pizlo]

Comment on attachment 208210 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:941
> +        if (isObjectSpeculation(forNode(node->child1()).m_type))
> +            m_state.setFoundConstants(true);

This should say:

if (isObjectBlah...) {
    forNode(node) = forNode(node->child1());
    setFoundConstants
    break;
}

Reason: if constant folding turns this into a node that does X, then this should do exactly X.  Here X = Identity, right?

[Mark Hahnenberg]

(In reply to comment #5)
> (From update of attachment 208210 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208210&action=review
> 
> > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:941
> > +        if (isObjectSpeculation(forNode(node->child1()).m_type))
> > +            m_state.setFoundConstants(true);
> 
> This should say:
> 
> if (isObjectBlah...) {
>     forNode(node) = forNode(node->child1());
>     setFoundConstants
>     break;
> }
> 
> Reason: if constant folding turns this into a node that does X, then this should do exactly X.  Here X = Identity, right?

Along the same lines, if we haven't proven that node->child1() is SpecObject then we should filter the result of the current node on SpecObject (e.g. filter(forNode(node), SpecObject)) rather than just unconditionally setting the type, correct?

[Filip Pizlo]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 208210 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=208210&action=review
> > 
> > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:941
> > > +        if (isObjectSpeculation(forNode(node->child1()).m_type))
> > > +            m_state.setFoundConstants(true);
> > 
> > This should say:
> > 
> > if (isObjectBlah...) {
> >     forNode(node) = forNode(node->child1());
> >     setFoundConstants
> >     break;
> > }
> > 
> > Reason: if constant folding turns this into a node that does X, then this should do exactly X.  Here X = Identity, right?
> 
> Along the same lines, if we haven't proven that node->child1() is SpecObject then we should filter the result of the current node on SpecObject (e.g. filter(forNode(node), SpecObject)) rather than just unconditionally setting the type, correct?

Why would you filter the *result*?  Filtering is what you do to your children.  As far as I can tell you just know that the node returns one of its two children, whichever one happens to be an object.  The easiest way to convey that is to say that it returns SpecObject, as you do now.  Except of course when you're converting it to Identity.

[Filip Pizlo]

Comment on attachment 208210 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:3258
> +        m_jit.move(op1PayloadGPR, thisGPR);

Are you mutating an operand GPR?  Isn't that wrong?

[Mark Hahnenberg]

Created attachment 208222 [details]
Patch

[Geoffrey Garen]

I wonder if we should change the bytecode to make get_object_or_this separate from ret.

[Filip Pizlo]

(In reply to comment #10)
> I wonder if we should change the bytecode to make get_object_or_this separate from ret.

Could be useful.  Though, at this point, it probably wouldn't simplify the DFG.  Are there some other simplifications you're thinking of?

[Filip Pizlo]

(In reply to comment #11)
> (In reply to comment #10)
> > I wonder if we should change the bytecode to make get_object_or_this separate from ret.
> 
> Could be useful.  Though, at this point, it probably wouldn't simplify the DFG.  Are there some other simplifications you're thinking of?

Hmmm, actually, what would be most natural is if the "is object" check was expressible in bytecode.  Then ret_object_or_this could just be a branch around some returns.

[Geoffrey Garen]

> Hmmm, actually, what would be most natural is if the "is object" check was expressible in bytecode.  Then ret_object_or_this could just be a branch around some returns.

Yeah, I like that.