Summary: | [JSC] ValueAdd, VaueSub, ValueMul, Inc, Dec should say SpecBigInt32 prediction based on ArithProfile | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2020-04-25 19:54:32 PDT
Created attachment 397608 [details]
Patch
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. I'll do Inc / Dec too. Created attachment 397609 [details]
Patch
Created attachment 397610 [details]
Patch
Created attachment 397611 [details]
Patch
Created attachment 397612 [details]
Patch
Committed r260730: <https://trac.webkit.org/changeset/260730> 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 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 |