Bug 210892 - [JSC] DFG compare should speculate BigInt well
Summary: [JSC] DFG compare should speculate BigInt well
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-22 19:28 PDT by Yusuke Suzuki
Modified: 2020-04-24 13:11 PDT (History)
7 users (show)

See Also:

Attachments
Patch (11.88 KB, patch)
2020-04-22 20:06 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (31.81 KB, patch)
2020-04-22 23:25 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (32.47 KB, patch)
2020-04-23 10:25 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (35.86 KB, patch)
2020-04-23 14:47 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (64.71 KB, patch)
2020-04-24 00:47 PDT, Yusuke Suzuki 		sbarati: review+
Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-04-22 19:28:09 PDT 
We are emitting DoubleRep(BigInt32) in FixupPhase and exiting.
Comment 1 Yusuke Suzuki 2020-04-22 20:06:06 PDT 
Created attachment 397308 [details]
Patch
Comment 2 Yusuke Suzuki 2020-04-22 23:25:23 PDT 
Created attachment 397323 [details]
Patch

WIP
Comment 3 Yusuke Suzuki 2020-04-23 10:25:21 PDT 
Created attachment 397357 [details]
Patch
Comment 4 Yusuke Suzuki 2020-04-23 14:47:33 PDT 
Created attachment 397387 [details]
Patch

WIP
Comment 5 Yusuke Suzuki 2020-04-24 00:47:43 PDT 
Created attachment 397433 [details]
Patch
Comment 6 Saam Barati 2020-04-24 11:30:56 PDT 
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?
Comment 7 Yusuke Suzuki 2020-04-24 12:09:24 PDT 
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.
Comment 8 Yusuke Suzuki 2020-04-24 12:19:28 PDT 
Committed r260660: <https://trac.webkit.org/changeset/260660>
Comment 9 Radar WebKit Bug Importer 2020-04-24 12:20:19 PDT 
<rdar://problem/62330722>
Comment 10 Yusuke Suzuki 2020-04-24 13:11:13 PDT 
Committed r260664: <https://trac.webkit.org/changeset/260664>