Summary: | B3 should support tuple types | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||||||||||||||||||
Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, fpizlo, mark.lam, msaboff, rniwa, 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=200411 | ||||||||||||||||||||||||
Attachments: |
|
Description
Keith Miller
2019-07-31 17:27:54 PDT
Created attachment 375278 [details]
Patch
Created attachment 375289 [details]
Patch
Comment on attachment 375289 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375289&action=review > Source/JavaScriptCore/b3/B3Type.h:39 > +static constexpr uint32_t tupleFlag = static_cast<uint32_t>(std::numeric_limits<int32_t>::min()); why not "1 << 31" > Source/JavaScriptCore/b3/B3Value.h:481 > +#ifdef NDEBUG > default: > break; > +#endif why? Comment on attachment 375289 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375289&action=review >> Source/JavaScriptCore/b3/B3Type.h:39 >> +static constexpr uint32_t tupleFlag = static_cast<uint32_t>(std::numeric_limits<int32_t>::min()); > > why not "1 << 31" I can do that if you think it's clearer. Is that better than `1 << bitSize<int32_t>() - 1;`? (I don't think we have a bitSize function but I could add one to MathExtras.h) >> Source/JavaScriptCore/b3/B3Value.h:481 >> +#endif > > why? So that you get a build error if you are missing a case in debug but corrupt opcodes still are handled in release. We do this elsewhere. Created attachment 375323 [details]
Patch
Comment on attachment 375323 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375323&action=review Looks totally sensible. Now if only it would apply to trunk, build, pass tests, etc. > Source/JavaScriptCore/b3/B3ExtractValue.h:40 > + int32_t offset() const { return m_value; } Can you pick either calling it offset or something else and be consistent? Seems like index is better. Offset suggests bytes to me. Created attachment 375325 [details]
Patch for landing
Created attachment 375326 [details]
Patch for landing
Created attachment 375327 [details]
Patch for landing
Comment on attachment 375323 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375323&action=review >> Source/JavaScriptCore/b3/B3ExtractValue.h:40 >> + int32_t offset() const { return m_value; } > > Can you pick either calling it offset or something else and be consistent? > > Seems like index is better. Offset suggests bytes to me. Sounds reasonable. I made the change. Comment on attachment 375327 [details] Patch for landing Attachment 375327 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12847723 Number of test failures exceeded the failure limit. Created attachment 375335 [details]
Archive of layout-test-results from ews102 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 375327 [details] Patch for landing Attachment 375327 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12847710 Number of test failures exceeded the failure limit. Created attachment 375337 [details]
Archive of layout-test-results from ews117 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 375355 [details]
Patch
Created attachment 375440 [details]
Patch
Committed r248178: <https://trac.webkit.org/changeset/248178> Comment on attachment 375440 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375440&action=review r=me too (even though I'm a bit late), some minor comments and one question on testing. The patch looks really solid and reasonable to me. > Source/JavaScriptCore/ChangeLog:18 > + there is a new Opcode, Extract, that takes yields the some, fixed, > + entry from a tuple value. Extract would be the only other change "the some, fixed, entry" => "some fixed entry" > Source/JavaScriptCore/ChangeLog:28 > + Tuples, we instead use a two new HashTables for Value->Tmp(s) and "a two" => "two" > Source/JavaScriptCore/b3/B3Opcode.h:300 > + // This is a projection out of a tuple. Currently only patchpoints can generate a tuple. It's assumumed that > + // each entry in a tuple has a fixed Numeric B3 Type (i.e. not Void or Tuple). Not true. Get and Phi's can also return a tuple > Source/JavaScriptCore/b3/B3PatchpointSpecial.cpp:120 > + unsigned returnCount = code().proc().returnCount(type); nit: Kind of a weird name both for local variable and function. Why not "numberOfValues" or something? > Source/JavaScriptCore/b3/B3Validate.cpp:76 > Can you also add a validation rule that look at each child C for each node N. If C is a tuple, then N must be a Patchpoint, Get, Set, Phi, Upsilon > Source/JavaScriptCore/b3/testb3_1.cpp:907 > + for (unsigned i = 1; i <= 2; ++i) { Why? This looks wrong. If a specific test is failing, just skip that test in O0. Comment on attachment 375440 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375440&action=review >> Source/JavaScriptCore/b3/B3Validate.cpp:76 >> > > Can you also add a validation rule that look at each child C for each node N. If C is a tuple, then N must be a Patchpoint, Get, Set, Phi, Upsilon This is wrong, the list should just be: Get, Phi, Patchpoint. (Upislon and Set don't return values, hence can't be a use.) Comment on attachment 375440 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375440&action=review >> Source/JavaScriptCore/b3/B3Opcode.h:300 >> + // each entry in a tuple has a fixed Numeric B3 Type (i.e. not Void or Tuple). > > Not true. Get and Phi's can also return a tuple Oh, maybe this is phrased poorly. I was thinking more along the lines of origin point. but I'll change it to include Get and Phi. >>> Source/JavaScriptCore/b3/B3Validate.cpp:76 >>> >> >> Can you also add a validation rule that look at each child C for each node N. If C is a tuple, then N must be a Patchpoint, Get, Set, Phi, Upsilon > > This is wrong, the list should just be: > > Get, Phi, Patchpoint. (Upislon and Set don't return values, hence can't be a use.) I'm not sure how valuable that is the only consumer of Tuple is Extract, Set, and, Upsilon. I think every opcode already checks the result type of their children. (or at least they should) >> Source/JavaScriptCore/b3/testb3_1.cpp:907 >> + for (unsigned i = 1; i <= 2; ++i) { > > Why? This looks wrong. If a specific test is failing, just skip that test in O0. Crap, that got added while I was fixing existing b3 tests... will revert. Addressed comments in https://bugs.webkit.org/show_bug.cgi?id=200411 |