| 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: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Tadeu Zagallo
2019-06-04 02:07:02 PDT
Created attachment 371256 [details]
Patch
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? 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. 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? (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. Created attachment 371309 [details]
Patch for landing
Comment on attachment 371309 [details] Patch for landing Clearing flags on attachment: 371309 Committed r246075: <https://trac.webkit.org/changeset/246075> All reviewed patches have been landed. Closing bug. *** Bug 197956 has been marked as a duplicate of this bug. *** |