| Summary: | Fix ArrayMode nodes after r261260 | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||
| Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | ews-watchlist, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=211531 | ||||||
| Attachments: |
|
||||||
|
Description
Keith Miller
2020-05-06 18:58:33 PDT
Created attachment 398695 [details]
Patch
Comment on attachment 398695 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398695&action=review r=me with nits > Source/JavaScriptCore/dfg/DFGClobberize.h:162 > + // all nodes with an ArrayMode can clobber top. We make an exception for CheckArray since it has no effects. > + if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode() && node->op() != CheckArray) > return clobberTop(); Can we list up all ArrayMode nodes here not to forget adding some nodes? Like this. if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) { switch (node->op()) { case GetIndexedPropertyStorage: ... break; case CheckArray: return clobberTop(); default: DFG_ASSERT(...); } } Do we need to include CheckArrayOrEmpty too? Comment on attachment 398695 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398695&action=review >> Source/JavaScriptCore/dfg/DFGClobberize.h:162 >> return clobberTop(); > > Can we list up all ArrayMode nodes here not to forget adding some nodes? > > Like this. > > if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) { > switch (node->op()) { > case GetIndexedPropertyStorage: > ... > break; > case CheckArray: > return clobberTop(); > default: > DFG_ASSERT(...); > } > } > > Do we need to include CheckArrayOrEmpty too? Yusuke, I think you meant the opposite: if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) { switch (node->op()) { case GetIndexedPropertyStorage: ... return clobberTop(); case CheckArray: break; default: DFG_ASSERT(...); } } Comment on attachment 398695 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398695&action=review >>> Source/JavaScriptCore/dfg/DFGClobberize.h:162 >>> return clobberTop(); >> >> Can we list up all ArrayMode nodes here not to forget adding some nodes? >> >> Like this. >> >> if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) { >> switch (node->op()) { >> case GetIndexedPropertyStorage: >> ... >> break; >> case CheckArray: >> return clobberTop(); >> default: >> DFG_ASSERT(...); >> } >> } >> >> Do we need to include CheckArrayOrEmpty too? > > Yusuke, I think you meant the opposite: > > if (graph.m_planStage < PlanStage::AfterFixup && node->hasArrayMode()) { > switch (node->op()) { > case GetIndexedPropertyStorage: > ... > return clobberTop(); > case CheckArray: > break; > default: > DFG_ASSERT(...); > } > } Oops, right. I reverted r261260 in https://trac.webkit.org/changeset/261293 to address test failures. This should be combined with the original patch and landed at once. Committed r261313: <https://trac.webkit.org/changeset/261313> |