Bug 202187 – [JSC] Date functions should have intrinsic

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

[Yusuke Suzuki]

It is super easy. Affects on JetStream2/date-format-xparb-SP.

[Yusuke Suzuki]

Created attachment 382045 [details]
Patch

[Yusuke Suzuki]

Created attachment 382046 [details]
Patch

[Yusuke Suzuki]

Created attachment 382048 [details]
Patch

[Mark Lam]

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

[Yusuke Suzuki]

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 :)

[Yusuke Suzuki]

Created attachment 382154 [details]
Patch

[Yusuke Suzuki]

Created attachment 382159 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/56726985>

[Yusuke Suzuki]

Created attachment 382273 [details]
Patch

[Yusuke Suzuki]

Created attachment 382275 [details]
Patch

[Keith Miller]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r251826: <https://trac.webkit.org/changeset/251826>