Bug 200327 – B3 should support tuple types

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

B3 should support tuple types

[Keith Miller]

Created attachment 375278 [details]
Patch

[Keith Miller]

Created attachment 375289 [details]
Patch

[Saam Barati]

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?

[Keith Miller]

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.

[Keith Miller]

Created attachment 375323 [details]
Patch

[Filip Pizlo]

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.

[Keith Miller]

Created attachment 375325 [details]
Patch for landing

[Keith Miller]

Created attachment 375326 [details]
Patch for landing

[Keith Miller]

Created attachment 375327 [details]
Patch for landing

[Keith Miller]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Keith Miller]

Created attachment 375355 [details]
Patch

[Keith Miller]

Created attachment 375440 [details]
Patch

[Keith Miller]

Committed r248178: <https://trac.webkit.org/changeset/248178>

[Radar WebKit Bug Importer]

<rdar://problem/53878615>

[Saam Barati]

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.

[Saam Barati]

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.)

[Keith Miller]

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.

[Keith Miller]

Addressed comments in https://bugs.webkit.org/show_bug.cgi?id=200411