Bug 200327 - B3 should support tuple types
Summary: B3 should support tuple types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-31 17:27 PDT by Keith Miller
Modified: 2019-08-02 17:05 PDT (History)
9 users (show)

See Also:


Attachments
Patch (115.43 KB, patch)
2019-07-31 18:58 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (116.92 KB, patch)
2019-07-31 21:15 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (121.61 KB, patch)
2019-08-01 10:50 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (121.89 KB, patch)
2019-08-01 11:12 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (121.88 KB, patch)
2019-08-01 11:18 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (121.88 KB, patch)
2019-08-01 11:20 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (1.06 MB, application/zip)
2019-08-01 12:28 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-highsierra (1.41 MB, application/zip)
2019-08-01 12:43 PDT, Build Bot
no flags Details
Patch (122.39 KB, patch)
2019-08-01 15:16 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (123.49 KB, patch)
2019-08-02 12:39 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2019-07-31 17:27:54 PDT
B3 should support tuple types
Comment 1 Keith Miller 2019-07-31 18:58:11 PDT
Created attachment 375278 [details]
Patch
Comment 2 Keith Miller 2019-07-31 21:15:11 PDT
Created attachment 375289 [details]
Patch
Comment 3 Saam Barati 2019-08-01 10:06:13 PDT
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 4 Keith Miller 2019-08-01 10:41:54 PDT
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.
Comment 5 Keith Miller 2019-08-01 10:50:34 PDT
Created attachment 375323 [details]
Patch
Comment 6 Filip Pizlo 2019-08-01 10:56:32 PDT
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.
Comment 7 Keith Miller 2019-08-01 11:12:45 PDT
Created attachment 375325 [details]
Patch for landing
Comment 8 Keith Miller 2019-08-01 11:18:57 PDT
Created attachment 375326 [details]
Patch for landing
Comment 9 Keith Miller 2019-08-01 11:20:15 PDT
Created attachment 375327 [details]
Patch for landing
Comment 10 Keith Miller 2019-08-01 11:21:08 PDT
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 11 Build Bot 2019-08-01 12:28:43 PDT
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.
Comment 12 Build Bot 2019-08-01 12:28:45 PDT
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 13 Build Bot 2019-08-01 12:43:30 PDT
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.
Comment 14 Build Bot 2019-08-01 12:43:32 PDT
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
Comment 15 Keith Miller 2019-08-01 15:16:17 PDT
Created attachment 375355 [details]
Patch
Comment 16 Keith Miller 2019-08-02 12:39:07 PDT
Created attachment 375440 [details]
Patch
Comment 17 Keith Miller 2019-08-02 14:02:12 PDT
Committed r248178: <https://trac.webkit.org/changeset/248178>
Comment 18 Radar WebKit Bug Importer 2019-08-02 14:03:18 PDT
<rdar://problem/53878615>
Comment 19 Saam Barati 2019-08-02 15:23:05 PDT
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 20 Saam Barati 2019-08-02 15:24:15 PDT
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 21 Keith Miller 2019-08-02 16:58:02 PDT
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.
Comment 22 Keith Miller 2019-08-02 17:05:40 PDT
Addressed comments in https://bugs.webkit.org/show_bug.cgi?id=200411