Bug 202187 - [JSC] Date functions should have intrinsic
Summary: [JSC] Date functions should have intrinsic
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: 203550
  Show dependency treegraph
 
Reported: 2019-09-25 00:30 PDT by Yusuke Suzuki
Modified: 2019-10-30 17:37 PDT (History)
12 users (show)

See Also:


Attachments
Patch (108.12 KB, patch)
2019-10-28 00:17 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (108.09 KB, patch)
2019-10-28 00:21 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (121.84 KB, patch)
2019-10-28 00:55 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (122.37 KB, patch)
2019-10-28 20:36 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (122.74 KB, patch)
2019-10-28 22:28 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (123.75 KB, patch)
2019-10-29 20:38 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (123.95 KB, patch)
2019-10-29 20:49 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 2019-09-25 00:30:36 PDT
It is super easy. Affects on JetStream2/date-format-xparb-SP.
Comment 1 Yusuke Suzuki 2019-10-28 00:17:52 PDT
Created attachment 382045 [details]
Patch
Comment 2 Yusuke Suzuki 2019-10-28 00:21:41 PDT
Created attachment 382046 [details]
Patch
Comment 3 Yusuke Suzuki 2019-10-28 00:55:52 PDT
Created attachment 382048 [details]
Patch
Comment 4 Mark Lam 2019-10-28 13:22:15 PDT
Comment on attachment 382048 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:4079
> +    case DateGetInt: {
> +        setNonCellTypeForNode(node, SpecBytecodeNumber);
> +        break;
> +    }
> +
> +    case DateGetFloat: {
> +        setNonCellTypeForNode(node, SpecFullDouble);
> +        break;
> +    }

It's weird to call these DateGetInt and DateGetFloat which suggests that the distinction is between Int and Float, when the real distinction is between JSNumber (int or double) and MachineDouble.  What do you think of renaming these as:
DateGetInt => DateGetJSNumber or DateGetJSEncodeableNumber
DateGetFloat = DateGetMachineDouble
Comment 5 Yusuke Suzuki 2019-10-28 20:31:49 PDT
Comment on attachment 382048 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:4079
>> +    }
> 
> It's weird to call these DateGetInt and DateGetFloat which suggests that the distinction is between Int and Float, when the real distinction is between JSNumber (int or double) and MachineDouble.  What do you think of renaming these as:
> DateGetInt => DateGetJSNumber or DateGetJSEncodeableNumber
> DateGetFloat = DateGetMachineDouble

Sounds fine to me. Changed :)
Comment 6 Yusuke Suzuki 2019-10-28 20:36:10 PDT
Created attachment 382154 [details]
Patch
Comment 7 Yusuke Suzuki 2019-10-28 22:28:07 PDT
Created attachment 382159 [details]
Patch
Comment 8 Radar WebKit Bug Importer 2019-10-29 16:48:17 PDT
<rdar://problem/56726985>
Comment 9 Yusuke Suzuki 2019-10-29 20:38:25 PDT
Created attachment 382273 [details]
Patch
Comment 10 Yusuke Suzuki 2019-10-29 20:49:40 PDT
Created attachment 382275 [details]
Patch
Comment 11 Keith Miller 2019-10-30 16:34:39 PDT
Comment on attachment 382275 [details]
Patch

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

r=me with comments.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:4072
> +        setNonCellTypeForNode(node, SpecBytecodeNumber);

Can say this is SpecInt32Only | SpecDoublePureNaN instead of SpecBytecodeNumber?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3203
> +            setResult(addToGraph(DateGetMachineDouble, OpInfo(intrinsic), OpInfo(), base));

Can we rename this to date get time?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3229
> +            setResult(addToGraph(DateGetJSNumber, OpInfo(intrinsic), OpInfo(prediction), base));

Hmm, can we rename this to DateGetInt32OrNaN? This implies that it can return any JSNumber but it can only vend an int32 or NaN.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:14029
> +

Please remove.
Comment 12 Yusuke Suzuki 2019-10-30 17:09:55 PDT
Comment on attachment 382275 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:4072
>> +        setNonCellTypeForNode(node, SpecBytecodeNumber);
> 
> Can say this is SpecInt32Only | SpecDoublePureNaN instead of SpecBytecodeNumber?

Sounds nice. Fixed.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3203
>> +            setResult(addToGraph(DateGetMachineDouble, OpInfo(intrinsic), OpInfo(), base));
> 
> Can we rename this to date get time?

Changed.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3229
>> +            setResult(addToGraph(DateGetJSNumber, OpInfo(intrinsic), OpInfo(prediction), base));
> 
> Hmm, can we rename this to DateGetInt32OrNaN? This implies that it can return any JSNumber but it can only vend an int32 or NaN.

Sounds better. Fixed.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:14029
>> +
> 
> Please remove.

Done.
Comment 13 Yusuke Suzuki 2019-10-30 17:37:11 PDT
Committed r251826: <https://trac.webkit.org/changeset/251826>