Bug 211038

Summary: [JSC] ValueAdd, VaueSub, ValueMul, Inc, Dec should say SpecBigInt32 prediction based on ArithProfile
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, sbarati, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch fpizlo: review+

Description Yusuke Suzuki 2020-04-25 19:54:32 PDT
...
Comment 1 Yusuke Suzuki 2020-04-25 21:17:57 PDT
Created attachment 397608 [details]
Patch
Comment 2 Yusuke Suzuki 2020-04-25 21:25:42 PDT
Comment on attachment 397608 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1112
> +        if (arithProfile->didObserveBigInt32())
> +            node->mergeFlags(NodeMayHaveBigInt32Result);
> +        if (arithProfile->didObserveHeapBigInt() || m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BigInt32Overflow))
> +            node->mergeFlags(NodeMayHaveHeapBigIntResult);

We are currently telling this but not using it yet for ValueDiv.
Comment 3 Yusuke Suzuki 2020-04-25 21:27:12 PDT
I'll do Inc / Dec too.
Comment 4 Yusuke Suzuki 2020-04-25 22:04:33 PDT
Created attachment 397609 [details]
Patch
Comment 5 Yusuke Suzuki 2020-04-25 22:48:56 PDT
Created attachment 397610 [details]
Patch
Comment 6 Yusuke Suzuki 2020-04-25 22:52:14 PDT
Created attachment 397611 [details]
Patch
Comment 7 Yusuke Suzuki 2020-04-25 22:55:36 PDT
Created attachment 397612 [details]
Patch
Comment 8 Yusuke Suzuki 2020-04-26 14:11:55 PDT
Committed r260730: <https://trac.webkit.org/changeset/260730>
Comment 9 Radar WebKit Bug Importer 2020-04-26 14:12:16 PDT
<rdar://problem/62408278>
Comment 10 Saam Barati 2020-04-27 18:36:16 PDT
Comment on attachment 397612 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:328
> +                // Shift can throw RangeError.

is this specified though? But maybe it's best to be consistent amongst the tiers.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:763
> +            if (Node::shouldSpeculateHeapBigInt(node->child1().node(), node->child2().node())) {

nice: I commented on this in Robin's patch but I'm glad you're fixing it here

> Source/JavaScriptCore/dfg/DFGNode.h:2880
> +    bool canSpeculateBigInt32(RareCaseProfilingSource source)

we're not using the rare case profiling for this, so do we really need it passed around?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4136
> +    // FIXME: Introduce another BigInt32 code generation: binary use kinds are BigIntUse32, but result is SpecAnyInt and accepting overflow.

Not super important, but it's not SpecAnyInt, it's:
SpecAnyInt => SpecAnyBigInt or SpecBigInt

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4986
> +    // FIXME: Introduce another BigInt32 code generation: binary use kinds are BigIntUse32, but result is SpecAnyInt and accepting overflow.

ditto

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2146
> +        // FIXME: Introduce another BigInt32 code generation: binary use kinds are BigIntUse32, but result is SpecAnyInt and accepting overflow.

ditto
Comment 11 Yusuke Suzuki 2020-04-27 18:43:38 PDT
Comment on attachment 397612 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:328
>> +                // Shift can throw RangeError.
> 
> is this specified though? But maybe it's best to be consistent amongst the tiers.

I think, to make it consistent with JSString etc. we should throw OOM error instead of RangeError.
And in that case, we can clear MustGenerate.
https://bugs.webkit.org/show_bug.cgi?id=211111

>> Source/JavaScriptCore/dfg/DFGNode.h:2880
>> +    bool canSpeculateBigInt32(RareCaseProfilingSource source)
> 
> we're not using the rare case profiling for this, so do we really need it passed around?

Currently not using it. But I left it for the future extension.

 168    // We always attempt to produce BigInt32 as much as possible. If we see HeapBigInt, we observed overflow.
 169    // FIXME: Extend this information by tiered profiling (from Baseline, DFG etc.).
 170    // https://bugs.webkit.org/show_bug.cgi?id=211039