|Summary:||[B3] Constants should be hoisted to the root block until moveConstants|
|Product:||WebKit||Reporter:||Robin Morisset <rmorisset>|
|Severity:||Normal||CC:||commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, sbarati, webkit-bug-importer|
|Version:||WebKit Nightly Build|
|Bug Depends on:|
|Bug Blocks:||197756, 197792|
Description Robin Morisset 2019-04-24 17:36:43 PDT
The moveConstants phase gathers and moves all constants based purely on their uses. So where they are located before that point is not relevant to anything. Keeping them orphaned (in other words not in a basic block) until then would have many benefits: - Makes all other B3 phases a bit faster, since they won't have to iterate over them. This might be significant as for example on JetStream2 27% of all B3Values at the beginning of B3 are constants (and up to 50% for some JS functions that reach FTL !) - MoveConstants won't have to replace the constants by Nops that stay allocated throughout Air (since there is no elimination of dead code after moveConstants). As 11% of all B3Values that reach Air are Nops (and as far as I can tell they all come from moveConstants), this might offer a small but not useless memory saving - By making basic blocks smaller this should enable tailDuplication and jump threading to trigger more often (since they are respectively limited to blocks of size <= 3 or blocks with nothing but a jump)
Comment 1 Robin Morisset 2019-04-29 16:05:20 PDT
After discussing this with Phil, he convinced me that the assumption that no value is orphaned is too strongly embedded throughout B3 to easily change. Instead we can simply hoist all constants to the root block in ReduceStrength, deduplicating them along the way. This won't provide the speed benefit from not iterating over them, but should provide both a small memory saving, and enable a bit more tail duplication.
Comment 3 EWS Watchlist 2019-04-29 17:45:43 PDT
Comment 4 Robin Morisset 2019-05-07 12:38:47 PDT
Created attachment 369315 [details] Patch Just rebased on ToT and fixed style nit.
Comment 5 Saam Barati 2019-05-07 12:50:19 PDT
Comment 6 Saam Barati 2019-05-07 12:51:46 PDT
Comment 7 Robin Morisset 2019-05-07 12:53:51 PDT
Comment 8 Robin Morisset 2019-05-07 12:54:17 PDT
Comment 9 Robin Morisset 2019-05-07 12:55:48 PDT
Comment 10 Robin Morisset 2019-05-07 12:56:51 PDT
(In reply to Robin Morisset from comment #9) Oops, I thought that option let me modify the patch in the browser, instead it dumped it as a comment. Sorry for the spam.
Comment 11 Robin Morisset 2019-05-07 13:02:36 PDT
Created attachment 369319 [details] Patch for landing Just tweaked the Changelog.
Comment 12 WebKit Commit Bot 2019-05-07 14:28:45 PDT
Comment on attachment 369319 [details] Patch for landing Clearing flags on attachment: 369319 Committed r245035: <https://trac.webkit.org/changeset/245035>
Comment 13 WebKit Commit Bot 2019-05-07 14:28:46 PDT
All reviewed patches have been landed. Closing bug.