Bug 200327

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>

<rdar://problem/53878615>

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