[webkit-reviews] review granted: [Bug 182470] [ESNext][BigInt] Add support for BigInt in SpeculatedType : [Attachment 336500] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Caio Lima <ticaiolima at gmail.com>'s
request for review:
Bug 182470: [ESNext][BigInt] Add support for BigInt in SpeculatedType
https://bugs.webkit.org/show_bug.cgi?id=182470

Attachment 336500: Patch

https://bugs.webkit.org/attachment.cgi?id=336500&action=review




--- Comment #26 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 336500
  --> https://bugs.webkit.org/attachment.cgi?id=336500
Patch

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

r=me with comments/suggestions.

Also, there is a place inside DFGAbstractInterpreterInlines for
CompareStrictEqual that you want to hook into. The line like:
 if (node->child1() == node->child2()) { ... }

Inside the ... you want to add a rule for BigInt speculations too.

> Source/JavaScriptCore/ChangeLog:15
> +	   patches is to implement BigInt equallity check directly in

equallity => equality

> Source/JavaScriptCore/bytecode/SpeculatedType.h:71
> +static const SpeculatedType SpecCellOther	      = 1ull << 26; // It's
definitely a JSCell but not a subclass of JSObject and definitely not a
JSString, BigInt or a Symbol.

JSString, BigInt or a Symbol => JSString, BigInt, or Symbol

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1471
> +	   if (isBigIntSpeculation(abstractChild.m_type)) {
> +	       setConstant(node,
*m_graph.freeze(m_vm.smallStrings.bigintString()));
> +	       break;
> +	   }

Do you have a test for this?

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2499
> +	       if (node->child1()->shouldSpeculateBigInt()) {

Do you have a test for this?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5868
> +    callOperation(operationCompareStrictEqCell, resultGPR, leftGPR,
rightGPR);

You're sure no exception will ever happen in this compare?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5871
> +    m_jit.and64(JITCompiler::TrustedImm32(1), resultGPR);

Why is this necessary?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6921
> +	       LValue left = lowJSValue(m_node->child1(),
ManualOperandSpeculation);
> +	       LValue right = lowJSValue(m_node->child2(),
ManualOperandSpeculation);

This is an anti pattern. You should add a lowBigInt function and use it instead
of speculate() above. See lowSymbol, lowObject, etc.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6927
> +	       ValueFromBlock fastResult = m_out.anchor(isEqualValue);

By making this the result, there is a chance B3/Air won't emit a fused compare
and branch. It's worth ensuring it does. You can always make fastResult just be
m_out.constInt32(1) if B3 doesn't emit a fused compare/branch. Actually, it's
probably just making this one since we know the result if the compare is true.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6928
> +	       m_out.branch(isEqualValue, rarely(continuation),
usually(slowPath));

If this is actually rarely/usually, you may want to give these blocks different
names.