|Summary:||Add SetCallee as DFG-Operation|
|Product:||WebKit||Reporter:||Dominik Inführ <dominik.infuehr>|
|Component:||New Bugs||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Normal||CC:||cgarcia, commit-queue, ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, rmorisset, sbarati, webkit-bug-importer, ysuzuki|
|Version:||WebKit Nightly Build|
Description Dominik Inführ 2018-04-13 03:10:03 PDT
Add SetCallee as DFG-Operation
Comment 3 EWS Watchlist 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]  Total errors found: 1 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.
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 7 Saam Barati 2018-04-13 08:22:23 PDT
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 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 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 17 Saam Barati 2018-04-20 15:00:09 PDT
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.