Bug 214956 - [JSC] Add B3::BottomTupleValue node
Summary: [JSC] Add B3::BottomTupleValue node
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: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-29 19:50 PDT by Yusuke Suzuki
Modified: 2020-07-30 11:21 PDT (History)
7 users (show)

See Also:


Attachments
Patch (25.19 KB, patch)
2020-07-29 19:58 PDT, Yusuke Suzuki
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-07-29 19:50:42 PDT
[JSC] Add B3::BottomTupleValue node
Comment 1 Yusuke Suzuki 2020-07-29 19:58:10 PDT
Created attachment 405542 [details]
Patch
Comment 2 Yusuke Suzuki 2020-07-29 19:58:12 PDT
<rdar://problem/65192877>
Comment 3 Keith Miller 2020-07-29 20:09:11 PDT
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 4 Yusuke Suzuki 2020-07-29 20:41:15 PDT
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!
Comment 5 Yusuke Suzuki 2020-07-29 21:42:53 PDT
Committed r265074: <https://trac.webkit.org/changeset/265074>
Comment 6 Yusuke Suzuki 2020-07-29 22:43:39 PDT
Committed r265076: <https://trac.webkit.org/changeset/265076>
Comment 7 Yusuke Suzuki 2020-07-30 11:21:04 PDT
Committed r265090: <https://trac.webkit.org/changeset/265090>