[webkit-reviews] review granted: [Bug 206182] Support an inlined representation in JSValue of small BigInts ("BigInt32") : [Attachment 396728] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Robin Morisset
<rmorisset at apple.com>'s request for review:
Bug 206182: Support an inlined representation in JSValue of small BigInts
("BigInt32")
https://bugs.webkit.org/show_bug.cgi?id=206182

Attachment 396728: Patch

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




--- Comment #29 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 396728
  --> https://bugs.webkit.org/attachment.cgi?id=396728
Patch

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

r=me with bug fixes I found.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1402
> +    GPRReg tempGPR = allocate();

Use GPRTemporary. `allocate()` and `unlock()` is very low-level concept to
implement `GPRTemporary`, `SpeculatedXXXOperand` etc.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1758
> +	   m_jit.move(op1.gpr(), result.gpr());
> +	   m_jit.unboxBigInt32(result.gpr());
> +	   m_jit.move(op2.gpr(), temp.gpr());
> +	   m_jit.unboxBigInt32(temp.gpr());
> +	   m_jit.compare64(condition, result.gpr(), temp.gpr(), result.gpr());

This is not correct. GPRTemporary result is using Reuse, this means that it can
be either of op1 / op2 registers.
Let's assume that result picks op2's register. Then, `m_jit.move(op1.gpr(),
result.gpr());` clobbers op2's register.

Rewrite it in this way. When using Reuse, we need to be extra careful. And
please define GPRRegs first instead of using gpr().

SpeculateBigInt32Operand op1(this, node->child1());
SpeculateBigInt32Operand op2(this, node->child2());
GPRTemporary result(this, Reuse, op1, op2);

GPRReg op1GPR = op1.gpr();
GPRReg op2GPR = op2.gpr();
GPRReg resultGPR = result.gpr();

if (condition == MacroAssembler::Equal || condition ==
MacroAssembler::NotEqual) {
    // No need to unbox the operands, since the tag bits are identical
    m_jit.compare64(condition, op1GPR, op2GPR, resultGPR);
} else {
    GPRTemporary temp(this);
    GPRReg tempGPR = temp.gpr();

    m_jit.move(op1GPR, tempGPR);
    m_jit.unboxBigInt32(tempGPR);
    m_jit.move(op2GPR, resultGPR);
    m_jit.unboxBigInt32(resultGPR);
    m_jit.compare64(condition, tempGPR, resultGPR, resultGPR);
}

// If we add a DataFormatBool, we should use it here.
m_jit.or32(TrustedImm32(JSValue::ValueFalse), resultGPR);
jsValueResult(resultGPR, m_currentNode, DataFormatJSBoolean);

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8746
> +	       speculateHeapBigIntUnknownWhetherCell(m_node->child2());

This looks incorrect. speculateHeapBigIntUnknownWhetherCell is static
characteristics, which filters the AbstractValue. Not a path-related
characteristics to FTL code.
If you execute speculateHeapBigIntUnknownWhetherCell in FTL code generation
part, regardless of which path is taken at runtime, this filters out
AbstractValue, which is wrong.
You need to manually filter out AbstractValue with AnyBigInt. Read
FTLLowerDFGToB3.cpp's m_interpreter.filter related code.
Whenever you use specualteXXX, you need to ensure that this is
not-runtime-related characteristics (this is effective for CompareStrictEq
code, not only for onlyLeftIsBigInt32 branch case.)
If there are the same issue, please fix.

And add FIXME about filtering out proven-type based if-check removal.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8752
> +	       speculateHeapBigIntUnknownWhetherCell(m_node->child1());

Ditto, this is wrong.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8768
> +	       speculateHeapBigIntUnknownWhetherCell(m_node->child2());

Ditto, this is wrong.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8777
> +	   }

Can you rewrite it with appendTo(xxx, yyy) style instead of appendTo(xxx)?
The other places are using `appendTo(xxx, yyy)` thing.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:10929
> +	   ValueFromBlock notCellResult = m_out.anchor(isBigInt32(value,
provenType(m_node->child1())));

Add FIXME to filter out type based on the previous check.

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

Why is this `speculate` & `lowJSValue` order swapped?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:17133
> +    LValue isNotAnyBigInt(LValue jsValue, SpeculatedType type = SpecFullTop)

Can you rewrite it with appendTo(xxx, yyy) style instead of appendTo(xxx)?
The other places are using `appendTo(xxx, yyy)` thing.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:17156
> +	   m_out.branch(isCell(jsValue, type), unsure(isCellCase),
unsure(isNotCellCase));

Add FIXME to filter out type based on the previous `isBigInt32()` check.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:17689
> +	   ValueFromBlock returnForCell = m_out.anchor(isHeapBigInt(value,
type));

Add FIXME to filter out type based on the previous `isBigInt32()` check.