Bug 210892 – [JSC] DFG compare should speculate BigInt well

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

We are emitting DoubleRep(BigInt32) in FixupPhase and exiting.

[Yusuke Suzuki]

Created attachment 397308 [details]
Patch

[Yusuke Suzuki]

Created attachment 397323 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 397357 [details]
Patch

[Yusuke Suzuki]

Created attachment 397387 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 397433 [details]
Patch

[Saam Barati]

Comment on attachment 397433 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397433&action=review

nice. r=me

> Source/JavaScriptCore/runtime/JSBigInt.cpp:2033
> +        return ComparisonResult::LessThan;

nit: I know this is proven given all other things, but maybe before this, add an
"ASSERT(!ySign);"

> Source/JavaScriptCore/runtime/JSBigInt.h:293
> +ALWAYS_INLINE JSBigInt::ComparisonResult swapBigIntCompareResult(JSBigInt::ComparisonResult comparisonResult)

name nit:
swap => invert

> Source/JavaScriptCore/runtime/JSBigInt.h:301
> +    default:
> +        return comparisonResult;

nit:

can we define all cases here, just so we get a compile error in the future?

> Source/JavaScriptCore/runtime/Operations.h:314
> +        JSValue bigIntValue = JSBigInt::stringToBigInt(globalObject, asString(primValue)->value(globalObject));

don't we need an exception check after ->value() on string?

[Yusuke Suzuki]

Comment on attachment 397433 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397433&action=review

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:2033
>> +        return ComparisonResult::LessThan;
> 
> nit: I know this is proven given all other things, but maybe before this, add an
> "ASSERT(!ySign);"

Sounds nice. Added.

>> Source/JavaScriptCore/runtime/JSBigInt.h:293
>> +ALWAYS_INLINE JSBigInt::ComparisonResult swapBigIntCompareResult(JSBigInt::ComparisonResult comparisonResult)
> 
> name nit:
> swap => invert

Fixed.

>> Source/JavaScriptCore/runtime/JSBigInt.h:301
>> +        return comparisonResult;
> 
> nit:
> 
> can we define all cases here, just so we get a compile error in the future?

Sounds good. Fixed.

[Yusuke Suzuki]

Committed r260660: <https://trac.webkit.org/changeset/260660>

[Radar WebKit Bug Importer]

<rdar://problem/62330722>

[Yusuke Suzuki]

Committed r260664: <https://trac.webkit.org/changeset/260664>