RESOLVED FIXED 211543
Fix ArrayMode nodes after r261260
https://bugs.webkit.org/show_bug.cgi?id=211543
Summary Fix ArrayMode nodes after r261260
Keith Miller
Reported 2020-05-06 18:58:33 PDT
Fix ArrayMode nodes after r261260
Attachments
Patch (2.73 KB, patch)
2020-05-06 19:00 PDT, Keith Miller
ysuzuki: review+
Keith Miller
Comment 1 2020-05-06 19:00:47 PDT
Yusuke Suzuki
Comment 2 2020-05-06 19:37:43 PDT
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?
Mark Lam
Comment 3 2020-05-06 20:29:56 PDT
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(...); } }
Yusuke Suzuki
Comment 4 2020-05-06 22:18:52 PDT
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.
Ryan Haddad
Comment 5 2020-05-07 09:20:42 PDT
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.
Keith Miller
Comment 6 2020-05-07 11:00:25 PDT
Radar WebKit Bug Importer
Comment 7 2020-05-07 11:01:30 PDT
Mark Lam
Comment 8 2020-05-07 11:14:42 PDT
Note You need to log in before you can comment on or make changes to this bug.