Bug 184582 - Add SetCallee as DFG-Operation
Summary: Add SetCallee as DFG-Operation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 180044 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-04-13 03:10 PDT by Dominik Inführ
Modified: 2018-05-01 08:43 PDT (History)
11 users (show)

See Also:


Attachments
Patch (13.38 KB, patch)
2018-04-13 04:34 PDT, Dominik Inführ
no flags Details | Formatted Diff | Diff
Patch (13.99 KB, patch)
2018-04-13 04:40 PDT, Dominik Inführ
no flags Details | Formatted Diff | Diff
Patch (13.99 KB, patch)
2018-04-13 06:10 PDT, Dominik Inführ
no flags Details | Formatted Diff | Diff
Patch (16.43 KB, patch)
2018-04-19 06:50 PDT, Dominik Inführ
no flags Details | Formatted Diff | Diff
Patch (16.89 KB, patch)
2018-04-20 11:05 PDT, Dominik Inführ
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominik Inführ 2018-04-13 03:10:03 PDT
Add SetCallee as DFG-Operation
Comment 1 Dominik Inführ 2018-04-13 04:34:04 PDT
Created attachment 337878 [details]
Patch
Comment 2 Dominik Inführ 2018-04-13 04:40:19 PDT
Created attachment 337879 [details]
Patch
Comment 3 Build Bot 2018-04-13 04:43:07 PDT
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.
Comment 4 Dominik Inführ 2018-04-13 06:10:18 PDT
Created attachment 337883 [details]
Patch
Comment 5 Saam Barati 2018-04-13 08:15:05 PDT
I know Robin was also working on exactly this bug
Comment 6 Saam Barati 2018-04-13 08:21:25 PDT
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.
Comment 7 Saam Barati 2018-04-13 08:22:23 PDT
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.
Comment 8 Dominik Inführ 2018-04-13 09:30:55 PDT
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
Comment 9 Saam Barati 2018-04-13 10:42:24 PDT
(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)
Comment 10 Dominik Inführ 2018-04-19 06:50:11 PDT
Created attachment 338321 [details]
Patch
Comment 11 Dominik Inführ 2018-04-19 07:00:13 PDT
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.
Comment 12 Robin Morisset 2018-04-20 07:47:06 PDT
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.
Comment 13 Dominik Inführ 2018-04-20 11:05:14 PDT
Created attachment 338440 [details]
Patch
Comment 14 Dominik Inführ 2018-04-20 11:09:59 PDT
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.
Comment 15 Filip Pizlo 2018-04-20 11:13:10 PDT
R=me, based on Robin’s approval.
Comment 16 Saam Barati 2018-04-20 14:59:52 PDT
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?
Comment 17 Saam Barati 2018-04-20 15:00:09 PDT
(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.
Comment 18 Dominik Inführ 2018-04-23 00:51:04 PDT
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);
Comment 19 Robin Morisset 2018-04-23 06:02:34 PDT
*** Bug 180044 has been marked as a duplicate of this bug. ***
Comment 20 Dominik Inführ 2018-05-01 02:33:58 PDT
Hi, is there anything left for me to do such that the patch can be merged? I think I've addressed Saam's question.
Comment 21 WebKit Commit Bot 2018-05-01 08:42:16 PDT
Comment on attachment 338440 [details]
Patch

Clearing flags on attachment: 338440

Committed r231195: <https://trac.webkit.org/changeset/231195>
Comment 22 WebKit Commit Bot 2018-05-01 08:42:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Radar WebKit Bug Importer 2018-05-01 08:43:17 PDT
<rdar://problem/39866759>