| Summary: | [JSC] Add B3::BottomTupleValue node | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||
| Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=214975 | ||||||
| Attachments: |
|
||||||
|
Description
Yusuke Suzuki
2020-07-29 19:50:42 PDT
Created attachment 405542 [details]
Patch
Comment on attachment 405542 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405542&action=review r=me with nits. > Source/JavaScriptCore/ChangeLog:10 > + While we support bottom values for usual types, we are not having bottom value for tuple type. So when replaceWithBottom is called, we Nit: we are not having bottom value => we do not have a bottom value. > Source/JavaScriptCore/ChangeLog:11 > + failed to replace Patchpoint producing tuple with bottom values. Nit: failed to replace Patchpoint producing tuple => fail to replace Patchpoints producing tuples > Source/JavaScriptCore/ChangeLog:13 > + This patch newly adds B3 BottomTupleValue, which is just generating BottomValue for tuple. We can extend it to generate arbitrary constant Nit: is just generating BottomValue => is just a BottomValue > Source/JavaScriptCore/ChangeLog:14 > + tuple values, but for now, we just support bottom tuple value case. We add a new node instead of generating patchpoint which generates bottom Nit: value case => values. > Source/JavaScriptCore/b3/B3Opcode.h:60 > + // Tuple filled with zeros. This appears when Tuple Patchpoints are replaced with Bottom values. > + BottomTuple, Can we update the Extract comment to note that BottomTuple can emit tuples. I would say we should update the HTML doc but I guess I never did that... whoops 😬 > Source/JavaScriptCore/b3/testb3_7.cpp:1630 > + kind, kind, kind, kind, kind, > + kind, kind, kind, kind, kind, > + kind, kind, kind, kind, kind, > + kind, kind, kind, kind, kind, > + kind, kind, kind, kind, kind, > + kind, kind, kind, kind, kind, > + kind, kind, kind, kind, kind, > + kind, kind, kind, kind, kind, > + kind, kind, kind, kind, kind, > + kind, kind, kind, kind, kind, lol. Comment on attachment 405542 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405542&action=review >> Source/JavaScriptCore/ChangeLog:10 >> + While we support bottom values for usual types, we are not having bottom value for tuple type. So when replaceWithBottom is called, we > > Nit: we are not having bottom value => we do not have a bottom value. Fixed. >> Source/JavaScriptCore/ChangeLog:11 >> + failed to replace Patchpoint producing tuple with bottom values. > > Nit: failed to replace Patchpoint producing tuple => fail to replace Patchpoints producing tuples Fixed. >> Source/JavaScriptCore/ChangeLog:13 >> + This patch newly adds B3 BottomTupleValue, which is just generating BottomValue for tuple. We can extend it to generate arbitrary constant > > Nit: is just generating BottomValue => is just a BottomValue Fixed. >> Source/JavaScriptCore/ChangeLog:14 >> + tuple values, but for now, we just support bottom tuple value case. We add a new node instead of generating patchpoint which generates bottom > > Nit: value case => values. Fixed. >> Source/JavaScriptCore/b3/B3Opcode.h:60 >> + BottomTuple, > > Can we update the Extract comment to note that BottomTuple can emit tuples. I would say we should update the HTML doc but I guess I never did that... whoops 😬 Fixed! Committed r265074: <https://trac.webkit.org/changeset/265074> Committed r265076: <https://trac.webkit.org/changeset/265076> Committed r265090: <https://trac.webkit.org/changeset/265090> |