Bug 211038 – [JSC] ValueAdd, VaueSub, ValueMul, Inc, Dec should say SpecBigInt32 prediction based on ArithProfile

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

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 397608 [details]
Patch

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

I'll do Inc / Dec too.

[Yusuke Suzuki]

Created attachment 397609 [details]
Patch

[Yusuke Suzuki]

Created attachment 397610 [details]
Patch

[Yusuke Suzuki]

Created attachment 397611 [details]
Patch

[Yusuke Suzuki]

Created attachment 397612 [details]
Patch

[Yusuke Suzuki]

Committed r260730: <https://trac.webkit.org/changeset/260730>

[Radar WebKit Bug Importer]

<rdar://problem/62408278>

[Saam Barati]

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

[Yusuke Suzuki]

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