Bug 184582 – Add SetCallee as DFG-Operation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dominik Inführ]

Add SetCallee as DFG-Operation

[Dominik Inführ]

Created attachment 337878 [details]
Patch

[Dominik Inführ]

Created attachment 337879 [details]
Patch

[EWS Watchlist]

Attachment 337879 [details] did not pass style-queue:


ERROR: JSTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dominik Inführ]

Created attachment 337883 [details]
Patch

[Saam Barati]

I know Robin was also working on exactly this bug

[Saam Barati]

Comment on attachment 337883 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2455
> +    case SetCallee:

Do we not model Callee is a variable?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1399
> +            addToGraph(SetCallee, OpInfo(bitwise_cast<intptr_t>(function)));

This doesn’t look completely right. You need to always do this if you’re looping back to the machine call frame (regardless of the variant being a constant value). Also, you need to do this anytime you loop back to an inline frame that has its callee in a stack slot.

[Saam Barati]

Comment on attachment 337883 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2455
>> +    case SetCallee:
> 
> Do we not model Callee is a variable?

Ignore this. I see that we don’t for the machine frame. My guess is we do for inline frames.

[Dominik Inführ]

I guess it should look like this?

addToGraph(SetCallee, OpInfo(bitwise_cast<intptr_t>(callVariant.function())));

if (stackEntry->m_inlineCallFrame)
    // update callee stack slot for inline frame

[Saam Barati]

(In reply to Dominik Inführ from comment #8)
> I guess it should look like this?
> 
> addToGraph(SetCallee,
> OpInfo(bitwise_cast<intptr_t>(callVariant.function())));
> 
> if (stackEntry->m_inlineCallFrame)
>     // update callee stack slot for inline frame

Why is your call variant guaranteed to be a constant callee? That should the the case. What you want to do is express it in a normal data flow edge in the IR

Like:
a: Foo
b: SetCallee(SomeUse:@a)

[Dominik Inführ]

Created attachment 338321 [details]
Patch

[Dominik Inführ]

I've now updated the patch. The callee is now set both for the inline call frame and the machine call frame using the callTargetNode.

I've also added a check for the case when an inline call frame isn't a closure, in this case the callee is a constant and therefore can't be set/changed.

[Robin Morisset]

I am not an official reviewer, so please wait for a review by someone more senior, but it looks good to me.

While reviewing your patch, I realized that the following lines:
```
// Currently we cannot do this optimisation for closures. The problem is that "emitFunctionChecks" which we use later is too coarse, only checking the executable
    // and not the value of captured variables.
    if (callVariant.isClosureCall())
        return false;
```
were wrong (not a source of crashes, but they disable the optimization in many cases for no good reason). The case I was trying to prevent was precisely the case where the callee needs to be changed (and I got it wrong, isClosureCall() does not do what I thought it would do).
You may either remove them in this patch, or I will make a separate patch that removes them.

[Dominik Inführ]

Created attachment 338440 [details]
Patch

[Dominik Inführ]

Thanks for looking into the patch! I was actually wondering about that code but then forgot it... I've now removed the check you mentioned, hope this is okay for you.

[Filip Pizlo]

R=me, based on Robin’s approval.

[Saam Barati]

Comment on attachment 338440 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1402
> +                setDirect(stackEntry->remapOperand(VirtualRegister(CallFrameSlot::callee)), callTargetNode, NormalSet);

Why setDirect here? Why not also emit that for the machine frame case instead of adding a SetCallee node?
Or is the SetCallee node needed for some reason?

[Saam Barati]

(In reply to Saam Barati from comment #16)
> Comment on attachment 338440 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338440&action=review
> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1402
> > +                setDirect(stackEntry->remapOperand(VirtualRegister(CallFrameSlot::callee)), callTargetNode, NormalSet);
> 
> Why setDirect here? Why not also emit that for the machine frame case
> instead of adding a SetCallee node?
> Or is the SetCallee node needed for some reason?

LGTM too in general. Just this one question.

[Dominik Inführ]

I've added SetCallee based on the already exisiting node SetArgumentCountIncludingThis. We can't use SetDirect (at least in its current form) since it only handles setting arguments and locals. Setting the callee with setDirect, invokes setArgument which fails on the following assertion:

        unsigned argument = operand.toArgument();
        ASSERT(argument < m_numArguments);

[Robin Morisset]

*** Bug 180044 has been marked as a duplicate of this bug. ***

[Dominik Inführ]

Hi, is there anything left for me to do such that the patch can be merged? I think I've addressed Saam's question.

[WebKit Commit Bot]

Comment on attachment 338440 [details]
Patch

Clearing flags on attachment: 338440

Committed r231195: <https://trac.webkit.org/changeset/231195>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39866759>