Bug 198520

Summary:

Argument elimination should check transitive dependents for interference

Product:

WebKit

Reporter:

Tadeu Zagallo <tzagallo>

Component:

JavaScriptCore

Assignee:

Tadeu Zagallo <tzagallo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, fpizlo, keith_miller, mark.lam, msaboff, rmorisset, saam, webkit-bug-importer, wuninsu, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch for landing	none

Tadeu Zagallo

Reported 2019-06-04 02:07:02 PDT

<rdar://problem/50863343>

Attachments
Patch (13.56 KB, patch) 2019-06-04 02:10 PDT, Tadeu Zagallo	no flags	Details Formatted Diff Diff
Patch for landing (13.38 KB, patch) 2019-06-04 11:24 PDT, Tadeu Zagallo	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Tadeu Zagallo

Comment 1 2019-06-04 02:10:30 PDT

Created attachment 371256 [details] Patch

Saam Barati

Comment 2 2019-06-04 10:22:04 PDT

Comment on attachment 371256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371256&action=review LGTM too, just one question > Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:609 > + if (node == candidate && inlineCallFrame == candidate->origin.semantic.inlineCallFrame()) Why this? Why not just break unconditionally when finding the candidate like before?

Tadeu Zagallo

Comment 3 2019-06-04 10:23:52 PDT

Comment on attachment 371256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371256&action=review >> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:609 >> + if (node == candidate && inlineCallFrame == candidate->origin.semantic.inlineCallFrame()) > > Why this? Why not just break unconditionally when finding the candidate like before? Because the current candidate can interfere with the transitive candidate, which actually happens with the test case included.

Saam Barati

Comment 4 2019-06-04 10:26:34 PDT

That seems weird. I thought the KillStack was the issue? Also, it’s weird we’re running clobberize on the pre-transformed node. Presumably they may have different effects?

Tadeu Zagallo

Comment 5 2019-06-04 10:44:06 PDT

(In reply to Saam Barati from comment #4) > That seems weird. I thought the KillStack was the issue? You're right, that's not necessary. > Also, it’s weird we’re running clobberize on the pre-transformed node. > Presumably they may have different effects? Do you mean prior to transformation? Nothing has been transformed yet at this point. The tricky thing is that transform has to happen top-dowm and interference is computed bottom-up.

Tadeu Zagallo

Comment 6 2019-06-04 11:24:57 PDT

Created attachment 371309 [details] Patch for landing

WebKit Commit Bot

Comment 7 2019-06-04 12:06:39 PDT

Comment on attachment 371309 [details] Patch for landing Clearing flags on attachment: 371309 Committed r246075: <https://trac.webkit.org/changeset/246075>

WebKit Commit Bot

Comment 8 2019-06-04 12:06:41 PDT

All reviewed patches have been landed. Closing bug.

Yusuke Suzuki

Comment 9 2020-04-14 09:44:09 PDT

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

Note You need to log in before you can comment on or make changes to this bug.