Bug 214956 – [JSC] Add B3::BottomTupleValue node

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[JSC] Add B3::BottomTupleValue node

[Yusuke Suzuki]

Created attachment 405542 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/65192877>

[Keith Miller]

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.

[Yusuke Suzuki]

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!

[Yusuke Suzuki]

Committed r265074: <https://trac.webkit.org/changeset/265074>

[Yusuke Suzuki]

Committed r265076: <https://trac.webkit.org/changeset/265076>

[Yusuke Suzuki]

Committed r265090: <https://trac.webkit.org/changeset/265090>