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

Fri Apr 10 13:11:00 PDT 2020

Yusuke Suzuki <ysuzuki at apple.com> has denied 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 395917: Patch

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

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

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

Looks nice! I commented several things since I found bugs.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1987
>	       // FIXME: Revisit this condition when introducing BigInt to JSC.

Remove this FIXME.

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:150
>		       // FIXME: Revisit this condition when introducing BigInt
to JSC.

Remove this FIXME.

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:534
>		   }

In prediction-propagation-phase, we are setting SpecBigInt prediction based on
mayHaveBigIntResult. But this is suboptimal since we blur HeapBigInt / BigInt32
difference here.
After that, fixup phase will emit AnyBigIntUse for the results of these nods.
Can you file a FIXME introducing NodeMayHaveBigInt32Result and propagating this
information well?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4038
> +	   // FIXME: should we pass an informative SpeculationRecovery?

Not necessary so long as your code is not modifying registers/values belonging
to DFG::Node. So, remove this FIXME.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4041
> +	   // FIXME: should I do anything to free tempGPR?

Not necessary. Remove this FIXME.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4146
> +	   // FIXME: should we pass an informative SpeculationRecovery?

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4149
> +	   // FIXME: should I do anything to free tempGPR?

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4212
> +    return;

This return is not necessary.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4782
> +    // FIXME: I don't think this is true, we should probably have some path
for AnyBigInt.

This is not something nice-to-have, rather, we should validate our code to
ensure it. Can you do that?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4798
> +    // FIXME: add a fast path, at least for BigInt32, but probably also for
HeapBigInt here.

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5000
> +	   // FIXME: should we pass an informative SpeculationRecovery?

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5003
> +	   // FIXME: should I do anything to free tempGPR?

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1402
> +    JITCompiler::Jump notCell = m_jit.branchIfNotCell(valueRegs);
> +    speculateHeapBigInt(edge, valueRegs.gpr());
> +    notCell.link(&m_jit);

I don't think this is good. speculateXXX can potentially (currently, not doing
so) does `JSValueOperand` / `SpeculateXXXOperand`. If you jump over these
things, which makes DFG register allocator confused, and cause type-confusion
bug.
Can you rewrite this with typeCheck?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1403
> +    (SpeculateBigInt32Operand(this, edge, ManualOperandSpeculation)).gpr();

This does not perform any checks since it is ManualOperandSpeculation, correct?
I think we need to have manual check of BigInt32 here.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1427
> +	       m_gprs.retain(gpr, virtualRegister, SpillOrderConstant);

Let's assert jsValue is BigInt32.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1434
> +	   m_jit.load64(JITCompiler::addressFor(virtualRegister), gpr);

Don't we need to check spillFormat?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1445
> +	   info.fillJSValue(*m_stream, gpr, DataFormatJS);
> +	   if (type & ~SpecBigInt32) {
> +	       speculationCheck(BadType, JSValueRegs(gpr), edge,
m_jit.branchIfNumber(gpr));
> +	       GPRReg tempGPR = allocate();
> +	       speculationCheck(BadType, JSValueRegs(gpr), edge,
m_jit.branchIfNotBigInt32KnownNotNumber(gpr, tempGPR));
> +	       unlock(tempGPR);
> +	   }
> +	   info.fillJSValue(*m_stream, gpr, DataFormatJSBigInt32);
> +	   return gpr;
> +    }

This part looks sharable with DataFormatJS.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1452
> +    case DataFormatBigInt32:
> +    case DataFormatJSBigInt32: {
> +	   GPRReg gpr = info.gpr();
> +	   m_gprs.lock(gpr);
> +	   return gpr;
> +    }

What is the difference between DataFormatBigInt32 and DataFormatJSBigInt32?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1465
> +    case DataFormatJS: {
> +	   GPRReg gpr = info.gpr();
> +	   m_gprs.lock(gpr);
> +	   if (type & ~SpecBigInt32) {
> +	       speculationCheck(BadType, JSValueRegs(gpr), edge,
m_jit.branchIfNumber(gpr));
> +	       GPRReg tempGPR = allocate();
> +	       speculationCheck(BadType, JSValueRegs(gpr), edge,
m_jit.branchIfNotBigInt32KnownNotNumber(gpr, tempGPR));
> +	       unlock(tempGPR);
> +	   }
> +	   info.fillJSValue(*m_stream, gpr, DataFormatJSBigInt32);
> +	   return gpr;
> +    }

Let's move this part just after DataFormatNone and put FALLTHROUGH in
DataFormatNone to share the code, as it is done in fillSpeculateInt32Internal.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4307
> +	   GPRTemporary result(this);
> +

I recommend defining 

GPRReg resultGPR = result.gpr();

instead of calling  result.gpr() multiple times since it allocates register at
the first call (so, side-effectful).

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4321
> +	   blessBoolean(result.gpr());
> +	   blessedBooleanResult(result.gpr(), node);

You can use unblessedBooleanResult(resultGPR, node).

> Source/JavaScriptCore/ftl/FTLCommonValues.cpp:61
> +#if USE(BIGINT32)

In FTL, I believe we do not need to have this if-def since FTL is only
accepting JSVALUE64 arch.

> Source/JavaScriptCore/ftl/FTLCommonValues.h:59
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2145
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2183
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2221
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2347
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3284
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3318
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3350
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3382
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3414
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7520
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:7532
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8645
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:10869
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:10897
> +#else // if !USE(BIGINT32)
> +    NO_RETURN_DUE_TO_CRASH ALWAYS_INLINE void compileIsBigInt()
> +    {
> +	   // If we are not dealing with BigInt32, we should just emit
IsCellWithType(HeapBigInt) instead.
> +	   RELEASE_ASSERT_NOT_REACHED();
> +    }
> +#endif

You do not need to have this.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:15407
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:16062
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:16134
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:16848
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:17052
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:17428
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:18127
> +#if USE(BIGINT32)

Ditto.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:977
> +    get(m_srcDst, t0)
> +    loadq [cfr, t0, 8], t1
> +    metadata(t2, t3)
> +    # srcDst in t1, metadata in t2
> +    # FIXME: the next line jumps to the slow path for BigInt32. We could
instead have a dedicated path in here for them.
> +    bqb t1, numberTag, .slow
> +    integerOperation(t1, .slow)
> +    orq numberTag, t1
> +    storeq t1, [cfr, t0, 8]
> +    updateArithProfile(ArithProfileInt)
> +    dispatch()

Can you fix this indentation?

> Source/JavaScriptCore/runtime/JSCJSValue.cpp:137
> +	   // FIXME: there must be a simpler way to find toThis for JSBigInt,
but I can't even find where it is defined.

In this case, JSCell::toThis is invoked. And it invokes toObject.