Bug 206182 – Support an inlined representation in JSValue of small BigInts ("BigInt32")

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

[Robin Morisset]

Created attachment 387545 [details]
WIP

The patch is not remotely ready to land yet, I am just opening this to have somewhere to backup the WIP.

[Robin Morisset]

Created attachment 389215 [details]
WIP_rebased

[Robin Morisset]

Created attachment 390604 [details]
WIP

still does not compile (what remains to be updated is part of DFGSpeculativeJIT.cpp and all of FTLLowerDFGToB3.cpp)

[Robin Morisset]

Created attachment 391370 [details]
WIP

Compiles, but still fails a couple dozen tests.

[Robin Morisset]

Created attachment 391448 [details]
WIP

Fixed a bunch of stupid bugs, still more to fix.

[Robin Morisset]

Created attachment 391711 [details]
WIP

Now passes all the tests, but still a bit rough in places, and lack a Changelog + perf evaluation.

[Robin Morisset]

Created attachment 392188 [details]
Patch

Still missing some performance numbers, but otherwise getting ready for review.
I will ask for review as soon as EWS is green.

[Robin Morisset]

Created attachment 392189 [details]
Patch

fix style nits, and fix a build issue on 32-bit.

[Robin Morisset]

Created attachment 392218 [details]
Patch

Fix build issues.

[Robin Morisset]

Created attachment 392237 [details]
Patch

Fix bug in DFGSpeculativeJIT

[Robin Morisset]

Created attachment 392238 [details]
Patch

Fix another stupid build issue.

[Robin Morisset]

Created attachment 392338 [details]
Patch

Fix various 32-bit build issues and warnings.

[Robin Morisset]

Created attachment 392341 [details]
Patch

fix CMake build.

[Yusuke Suzuki]

Comment on attachment 392341 [details]
Patch

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

Looking runtime part first.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:208
> +                nodeConstantOne = m_insertionSet.insertNode(m_indexInBlock, SpecHeapBigInt, JSConstant, node->origin, OpInfo(m_graph.freeze(vm().bigIntConstantOne.get())));

Let's rename VM::bigIntConstantOne to VM::heapBigIntConstantOne.

> 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()

Fix indentation.

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:127
> +        return JSValue::encode(JSValue(JSValue::JSBigInt32OrHeap, vm, static_cast<int64_t>(primitive.asDouble())));

Heap-allocation in JSValue constructor is not usual pattern. I would like to have static factory function in JSBigInt, which takes double and returns JSValue which is either JSBigInt* or BigInt32.

> Source/JavaScriptCore/runtime/Operations.cpp:83
> +        (void) left;
> +        (void) right;

Use UNUSED_PARAM.

> Source/JavaScriptCore/runtime/Operations.h:329
> +
> +

Remove this lines.

> Source/JavaScriptCore/runtime/Operations.h:540
> +        (void) left;
> +        (void) right;

Use `UNUSED_PARAM(left)`.

> Source/JavaScriptCore/runtime/Operations.h:562
> +        (void) left;
> +        (void) right;

Ditto.

> Source/JavaScriptCore/runtime/Operations.h:582
> +        (void) left;
> +        (void) right;

Ditto.

> Source/JavaScriptCore/runtime/Operations.h:742
> +#if USE(BIGINT32)
> +    JSBigInt* leftHeapBigInt = leftNumeric.isBigInt32() ? JSBigInt::createFrom(vm, leftNumeric.bigInt32AsInt32()) : leftNumeric.asHeapBigInt();
> +    JSBigInt* rightHeapBigInt = rightNumeric.isBigInt32() ? JSBigInt::createFrom(vm, rightNumeric.bigInt32AsInt32()) : rightNumeric.asHeapBigInt();
> +#else
> +    JSBigInt* leftHeapBigInt = leftNumeric.asHeapBigInt();
> +    JSBigInt* rightHeapBigInt = rightNumeric.asHeapBigInt();
> +#endif
> +
> +    if (isLeft)
> +        RELEASE_AND_RETURN(scope, JSBigInt::leftShift(globalObject, leftHeapBigInt, rightHeapBigInt));
> +    RELEASE_AND_RETURN(scope, JSBigInt::signedRightShift(globalObject, leftHeapBigInt, rightHeapBigInt));

My intuition is that, rightNumeric can be BitInt32 with high possibility.
Can you add FIXME here about adding `JSBigInt::leftShift(JSGlobalObject*, JSBigInt*, int32_t)` and JSBigInt::signedRightShift?
I think it would be possible that we have a lot of JSBigInt <binaryOp> BigInt32 case.

[Robin Morisset]

Created attachment 395605 [details]
Patch

rebased and applied Yusuke's comments (except for the indentation one, I don't see what was wrong with it).
Also fixed a couple of warnings that I had missed when compiling with USE(BIGINT32) 0.

[Robin Morisset]

Created attachment 395617 [details]
Patch

Add missing file back.

[Robin Morisset]

Created attachment 395917 [details]
Patch

Fixed an issue in the compilation of strict equality in the baseline tier on 32-bit architectures.

[Caio Lima]

For the record, there are 74 failures on ARMv7. I will have time to take a look at it next week. If this patch lands before that, do you mind open a bug and CC [e-mail]?

[Robin Morisset]

(In reply to Caio Lima from comment #17)
> For the record, there are 74 failures on ARMv7. I will have time to take a
> look at it next week. If this patch lands before that, do you mind open a
> bug and CC [e-mail]?

These failures are the main reason it has not yet landed. I have fixed one bug on 32-bit so far, but there is still visibly (at least) one more, that I am currently investigating.

[Yusuke Suzuki]

Comment on attachment 395917 [details]
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.

[Caio Lima]

Comment on attachment 395917 [details]
Patch

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

Nice work there! I've left some comments regarding what is causing 32-bits failures and other minor things.

> Source/JavaScriptCore/ChangeLog:29
> +        The latter is useful when we now that we are receiving BigInts, but speculation indicates a mix of heap-allocated and small (inlined) big-ints.

typo: "when we know that"

> Source/JavaScriptCore/ChangeLog:55
> +            return false;

Should this be `return "true"`?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:507
> +            ASSERT(USE_BIGINT32);

For example, this causes Debug build of 32-bits with JIT enabled to fail because `USE_BIGINT32` is not defined.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:743
> +            // FIXME: do we really need SpecString here for ValueSub? It seems like we only need it for ValueAdd.

I agree with this FIXME. `ValueSub` doesn't need `SpecString`.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1016
> +        // FIXME: why is this code not shared with ValueSub?

I think should move `ValeuSub` here and clear both `FIXME`.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4156
> +    // FIXME: why do compileValueAdd/compileValueMul use isKnownNotNumber but not ValueSub?

I don't remember this being an intentional omission. I think that including it would help generate less code when we know that math IC won't be useful.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:1920
> +                return createFrom(vm, static_cast<int64_t>(digit));

The issue with failures into ARMv7 (and all architectures using JSVALUE32_64) is due to a bug into `createFrom(VM, int64_t)`. The problem is that such function doesn't right trim the generated BigInt and on 32-bits and it always create a JSBigInt with `m_length = 2`. One possible solution that I think can be applied is to use `rightTrim` before returning the value from `createFrom(VM, int64_t)`, but this will be a bit painful to GC, since it will allocate 2 JSBigInts every time we call `createFrom(VM, int64_t)`. The other solution could check the range of input and properly set `m_length`.

> Source/JavaScriptCore/runtime/JSBigInt.h:92
> +        auto ptr = JSBigInt::createFrom(vm, value);

Ditto

> Source/JavaScriptCore/runtime/JSCJSValue.cpp:-395
> -        JSString* returnString = jsNontrivialString(vm, bigInt->toString(globalObject, 10));

It is not clear to me what is the major benefit if we remove this optimization. IIUC, we still can have 1n as a HeapBigInt if this was generated from a operation of 2 others HeapBigInt, right? Also, there is no `BigInt32` into 32-bits.

> Source/WTF/wtf/PlatformUse.h:144
> +#endif

I think we need a `#else` here, otherwise it will break compilation of `JSVALUE32_64` that refers it.

[Robin Morisset]

Thank you very much for finding the problem on 32-bit platforms! I fixed createFrom to use the right length.

Answers to your other comments below:

> > Source/JavaScriptCore/ChangeLog:29
> > +        The latter is useful when we now that we are receiving BigInts, but speculation indicates a mix of heap-allocated and small (inlined) big-ints.
> 
> typo: "when we know that"

Fixed.

> > Source/JavaScriptCore/ChangeLog:55
> > +            return false;
> 
> Should this be `return "true"`?

Yes, fixed.

> > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:507
> > +            ASSERT(USE_BIGINT32);
> 
> For example, this causes Debug build of 32-bits with JIT enabled to fail
> because `USE_BIGINT32` is not defined.

I replaced these by a RELEASE_ASSERT_NOT_REACHED() in the else path of #if USE(BIGINT32).
This seemed more idiomatic, as all of PlatformUse.h only has #define USE_FOO 1, never 0.

> > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:743
> > +            // FIXME: do we really need SpecString here for ValueSub? It seems like we only need it for ValueAdd.
> 
> I agree with this FIXME. `ValueSub` doesn't need `SpecString`.
> 
> > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1016
> > +        // FIXME: why is this code not shared with ValueSub?
> 
> I think should move `ValeuSub` here and clear both `FIXME`.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4156
> > +    // FIXME: why do compileValueAdd/compileValueMul use isKnownNotNumber but not ValueSub?
> 
> I don't remember this being an intentional omission. I think that including
> it would help generate less code when we know that math IC won't be useful.

Ok, I will do these simplifications and optimizations in another patch.

> > Source/JavaScriptCore/runtime/JSCJSValue.cpp:-395
> > -        JSString* returnString = jsNontrivialString(vm, bigInt->toString(globalObject, 10));
> 
> It is not clear to me what is the major benefit if we remove this
> optimization. IIUC, we still can have 1n as a HeapBigInt if this was
> generated from a operation of 2 others HeapBigInt, right? Also, there is no
> `BigInt32` into 32-bits.

I remember that I changed this code to fix a bug in debug builds, but I don't remember what exactly it was. I'll try putting it back in and testing again.

[Robin Morisset]

Thank you very much for the detailed review.
I answered your comments below (except for those that were just about redundant FIXMEs and style nits, which I removed/fixed).

> > 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?

Added a fixme.

> > 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?

There is already an ASSERT verifying it, and I tested it, this FIXME was just obsolete and I removed it.

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

This is not a correctness problem, since we emit the slow path which is correct for all types. It is just a possible (minor) perf issue, so I am leaving the FIXME for now.

 
> > 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?

I inlined speculateHeapBigInt there and added a comment based on what you say above. 

> > 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.

ManualOperandSpeculation simply says that it won't ASSERT that the useKind is BigInt32Use (which it won't be since it is AnyBigIntUse). It has no other effect, so no extra check is required.

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1427
> > +            m_gprs.retain(gpr, virtualRegister, SpillOrderConstant);
> 
> Let's assert jsValue is BigInt32.

Good idea.

> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1434
> > +        m_jit.load64(JITCompiler::addressFor(virtualRegister), gpr);
> 
> Don't we need to check spillFormat?

I don't understand this code well enough to be sure, but I just based it on fillSpeculateCell which does not use spillFormat at all. Considering that BigInt32 is a kind of JSValue (contrary to Int32), I expect the generic code should just work.

> 
> > 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.

It is not entirely trivial to share (as some of the code at the beginning is different), so I'm just adding a FIXME for now. Note that the situation is exactly the same for fillSpeculateCell

> 
> > 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?

Currently there is none, I just split it to follow the other types, such as DataFormatCell and DataFormatJSCell.

> 
> > 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.

I am not entirely sure it would keep the same behavior, as gpr is defined differently in these two cases. Since the original code in fillSpeculateCell does not do this, are you ok if I just leave a FIXME here that there might be chances to share some code?

> 
> > 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).

Good idea, done.

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

Thanks, done.

> > 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.

I want to keep the possibility to easily disable BIGINT32, even on the 64-bit platforms, it makes testing much easier.

> > 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.

If I remember correctly I had linker errors otherwise when I compile with USE_BIGINT32 = 0.
 
> > 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?

Done.

> 
> > 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.

Thanks for the pointer, I replaced the next line by just return "heapBigInt->toObject(globalObject);"

[Robin Morisset]

Created attachment 396582 [details]
Patch

[Robin Morisset]

Created attachment 396590 [details]
Patch

Fix the broken rebase

[Yusuke Suzuki]

Comment on attachment 396590 [details]
Patch

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

Commented :)

> Source/JavaScriptCore/bytecode/ArithProfile.cpp:57
> +    (void) tempGPR;

Use UNUSED_PARAM(tempGPR);.

> Source/JavaScriptCore/bytecode/DataFormat.h:47
> +    DataFormatBigInt32 = 8,

I think having DataFormatBigInt32 is confusing while there is not implementation exists. Add FIXME to mention that.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1407
> +    JSValueOperand value(this, edge, ManualOperandSpeculation);
> +    JSValueRegs valueRegs = value.jsValueRegs();
> +    JITCompiler::Jump notCell = m_jit.branchIfNotCell(valueRegs);
> +    // I inlined speculateHeapBigInt because it would be incorrect to call it here if it did JSValueOperand / SpeculateXXXOperand,
> +    // as it would confuse the DFG register allocator.
> +    DFG_TYPE_CHECK(JSValueSource::unboxedCell(valueRegs.gpr()), edge, ~SpecCellCheck | SpecHeapBigInt, m_jit.branchIfNotHeapBigInt(valueRegs.gpr()));
> +    notCell.link(&m_jit);
> +    (SpeculateBigInt32Operand(this, edge, ManualOperandSpeculation)).gpr();

This is not correct because this code will perform BigInt32 check even if we see HeapBigInt.
This should be written in this form I think.

JSValueRegs valueRegs = value.jsValueRegs();
JITCompiler::Jump notCell = m_jit.branchIfNotCell(valueRegs);
// I inlined speculateHeapBigInt because it would be incorrect to call it here if it did JSValueOperand / SpeculateXXXOperand,
// as it would confuse the DFG register allocator.
DFG_TYPE_CHECK(valueRegs, edge, ~SpecCellCheck | SpecHeapBigInt, m_jit.branchIfNotHeapBigInt(valueRegs.gpr()));
auto done = m_jit.jump();
notCell.link(&m_jit);
DFG_TYPE_CHECK(valueRegs, edge, SpecCellCheck | SpecBigInt32, m_jit.branchIfNotHeapBigInt(valueRegs.gpr()));
done.link(&m_jit);

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

We need to add  spillFormat == DataFormatJSInt32 case as it is done in fillSpeculateInt32Internal.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1447
> +            speculationCheck(BadType, JSValueRegs(gpr), edge, m_jit.branchIfNumber(gpr));
> +            GPRReg tempGPR = allocate();
> +            speculationCheck(BadType, JSValueRegs(gpr), edge, m_jit.branchIfNotBigInt32KnownNotNumber(gpr, tempGPR));
> +            unlock(tempGPR);

Let's write it in

CCallHelpers::JumpList failureCases;
GPRReg tempGPR = allocate();
failureCases.append(m_jit.branchIfNumber(gpr));
failureCases.append(m_jit.branchIfNotBigInt32KnownNotNumber(gpr, tempGPR));
speculationCheck(BadType, JSValueRegs(gpr), edge, failureCases);
unlock(tempGPR);

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1453
> +    case DataFormatBigInt32:

Let's make

case DataFormatBigInt32:
    DFG_CRASH(m_jit.graph(), m_currentNode, "Corrupt data format");


since there is no DataFormatBigInt32 implementation.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1467
> +            speculationCheck(BadType, JSValueRegs(gpr), edge, m_jit.branchIfNumber(gpr));
> +            GPRReg tempGPR = allocate();
> +            speculationCheck(BadType, JSValueRegs(gpr), edge, m_jit.branchIfNotBigInt32KnownNotNumber(gpr, tempGPR));
> +            unlock(tempGPR);

Let's write it in

CCallHelpers::JumpList failureCases;
GPRReg tempGPR = allocate();
failureCases.append(m_jit.branchIfNumber(gpr));
failureCases.append(m_jit.branchIfNotBigInt32KnownNotNumber(gpr, tempGPR));
speculationCheck(BadType, JSValueRegs(gpr), edge, failureCases);
unlock(tempGPR);

[Yusuke Suzuki]

Comment on attachment 396590 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1410
> +GPRReg SpeculativeJIT::fillSpeculateBigInt32(Edge edge)

Can we make fillSpeculateBigInt32 implementation further aligned to fillSpeculateInt32Internal?
Like, how we use switch-FALLTHROUGH etc.

[Robin Morisset]

 
> > Source/JavaScriptCore/bytecode/ArithProfile.cpp:57
> > +    (void) tempGPR;
> 
> Use UNUSED_PARAM(tempGPR);.

Done.

> > Source/JavaScriptCore/bytecode/DataFormat.h:47
> > +    DataFormatBigInt32 = 8,
> 
> I think having DataFormatBigInt32 is confusing while there is not
> implementation exists. Add FIXME to mention that.

Done.

> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1407
> > +    JSValueOperand value(this, edge, ManualOperandSpeculation);
> > +    JSValueRegs valueRegs = value.jsValueRegs();
> > +    JITCompiler::Jump notCell = m_jit.branchIfNotCell(valueRegs);
> > +    // I inlined speculateHeapBigInt because it would be incorrect to call it here if it did JSValueOperand / SpeculateXXXOperand,
> > +    // as it would confuse the DFG register allocator.
> > +    DFG_TYPE_CHECK(JSValueSource::unboxedCell(valueRegs.gpr()), edge, ~SpecCellCheck | SpecHeapBigInt, m_jit.branchIfNotHeapBigInt(valueRegs.gpr()));
> > +    notCell.link(&m_jit);
> > +    (SpeculateBigInt32Operand(this, edge, ManualOperandSpeculation)).gpr();
> 
> This is not correct because this code will perform BigInt32 check even if we
> see HeapBigInt.
> This should be written in this form I think.
> 
> JSValueRegs valueRegs = value.jsValueRegs();
> JITCompiler::Jump notCell = m_jit.branchIfNotCell(valueRegs);
> // I inlined speculateHeapBigInt because it would be incorrect to call it
> here if it did JSValueOperand / SpeculateXXXOperand,
> // as it would confuse the DFG register allocator.
> DFG_TYPE_CHECK(valueRegs, edge, ~SpecCellCheck | SpecHeapBigInt,
> m_jit.branchIfNotHeapBigInt(valueRegs.gpr()));
> auto done = m_jit.jump();
> notCell.link(&m_jit);
> DFG_TYPE_CHECK(valueRegs, edge, SpecCellCheck | SpecBigInt32,
> m_jit.branchIfNotHeapBigInt(valueRegs.gpr()));
> done.link(&m_jit);

Oops, thanks for finding this. Fixed.

> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1439
> > +        m_jit.load64(JITCompiler::addressFor(virtualRegister), gpr);
> 
> We need to add  spillFormat == DataFormatJSInt32 case as it is done in
> fillSpeculateInt32Internal.

I am still not clear on why that is needed, and why we can't just use the generic code, but I changed it based on fillSpeculateInt32Internal. Please have a look and tell me if it is what you wanted.
 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1447
> > +            speculationCheck(BadType, JSValueRegs(gpr), edge, m_jit.branchIfNumber(gpr));
> > +            GPRReg tempGPR = allocate();
> > +            speculationCheck(BadType, JSValueRegs(gpr), edge, m_jit.branchIfNotBigInt32KnownNotNumber(gpr, tempGPR));
> > +            unlock(tempGPR);
> 
> Let's write it in
> 
> CCallHelpers::JumpList failureCases;
> GPRReg tempGPR = allocate();
> failureCases.append(m_jit.branchIfNumber(gpr));
> failureCases.append(m_jit.branchIfNotBigInt32KnownNotNumber(gpr, tempGPR));
> speculationCheck(BadType, JSValueRegs(gpr), edge, failureCases);
> unlock(tempGPR);

Done.

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1453
> > +    case DataFormatBigInt32:
> 
> Let's make
> 
> case DataFormatBigInt32:
>     DFG_CRASH(m_jit.graph(), m_currentNode, "Corrupt data format");
> 
> 
> since there is no DataFormatBigInt32 implementation.

Done.

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1467
> > +            speculationCheck(BadType, JSValueRegs(gpr), edge, m_jit.branchIfNumber(gpr));
> > +            GPRReg tempGPR = allocate();
> > +            speculationCheck(BadType, JSValueRegs(gpr), edge, m_jit.branchIfNotBigInt32KnownNotNumber(gpr, tempGPR));
> > +            unlock(tempGPR);
> 
> Let's write it in
> 
> CCallHelpers::JumpList failureCases;
> GPRReg tempGPR = allocate();
> failureCases.append(m_jit.branchIfNumber(gpr));
> failureCases.append(m_jit.branchIfNotBigInt32KnownNotNumber(gpr, tempGPR));
> speculationCheck(BadType, JSValueRegs(gpr), edge, failureCases);
> unlock(tempGPR);

Done.

[Robin Morisset]

Created attachment 396728 [details]
Patch

applied Yusuke's suggestions, and fixed a style nit.

[Yusuke Suzuki]

Comment on attachment 396728 [details]
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.

[Mark Lam]

Comment on attachment 396728 [details]
Patch

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

Nice ChangeLog: very descriptive, and will make it easier .  Here are some comments while I continue to read the rest of the patch.

> JSTests/ChangeLog:8
> +        I improved various of the tests to give more informative error messages in the process of fixing them.

I suggest /various of the tests/several tests/

> JSTests/stress/big-int-right-shift-general.js:5
> +// Modified by Copyright (C) 2020 Apple.

I think you should remove the "Modified by" part.  Just say "Copyright (C) 2020 Apple. All rights reserved."  It is understood that the copyright is shared.

> JSTests/stress/compare-number-and-other.js:75
>  \ No newline at end of file

Can you make sure this I not here in the actual source?

> Source/JavaScriptCore/ChangeLog:21
> +        Note that in this patch we create such small BigInts when parsing BigInt constants, and most operations (e.g. Add or BitOr) produce a BigInt32 if both of their operands are BigInt32,

I think you mean when "parsing small BigInt constants".  I suggest rephrasing this as "we create BigInt32s when parsing small BigInt constants".  Using "BigInt32" instead of "such small BigInts" also keeps the terminology consistent with the rest of the sentence.

[Mark Lam]

Comment on attachment 396728 [details]
Patch

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

Publishing these comments before I forget and lose them.

> Source/JavaScriptCore/jit/AssemblyHelpers.h:956
> +        move(gpr, tempGPR);
> +        and64(TrustedImm32(JSValue::BigInt32Tag), tempGPR);
> +        return branch64(Equal, tempGPR, TrustedImm32(JSValue::BigInt32Tag));

I think you can use branchTest32(Zero, gpr, TrustedImm32(JSValue::BigInt32Tag)) here.

> Source/JavaScriptCore/jit/AssemblyHelpers.h:967
> +        move(gpr, tempGPR);
> +        and64(TrustedImm32(JSValue::BigInt32Tag), tempGPR);
> +        return branch64(NotEqual, tempGPR, TrustedImm32(JSValue::BigInt32Tag));

Can we use branchTest32(Zero, gpr, TrustedImm32(JSValue::BigInt32Tag)) instead?  I think that will generate more efficient code and doesn't need a tempGPR.

[Yusuke Suzuki]

Comment on attachment 396728 [details]
Patch

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

>> Source/JavaScriptCore/jit/AssemblyHelpers.h:956
>> +        return branch64(Equal, tempGPR, TrustedImm32(JSValue::BigInt32Tag));
> 
> I think you can use branchTest32(Zero, gpr, TrustedImm32(JSValue::BigInt32Tag)) here.

That sounds really nice because we could remove tempGPR...?

>> Source/JavaScriptCore/jit/AssemblyHelpers.h:967
>> +        return branch64(NotEqual, tempGPR, TrustedImm32(JSValue::BigInt32Tag));
> 
> Can we use branchTest32(Zero, gpr, TrustedImm32(JSValue::BigInt32Tag)) instead?  I think that will generate more efficient code and doesn't need a tempGPR.

Ditto, we could remove tempGPR use?

[Mark Lam]

Comment on attachment 396728 [details]
Patch

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

>>> Source/JavaScriptCore/jit/AssemblyHelpers.h:956
>>> +        return branch64(Equal, tempGPR, TrustedImm32(JSValue::BigInt32Tag));
>> 
>> I think you can use branchTest32(Zero, gpr, TrustedImm32(JSValue::BigInt32Tag)) here.
> 
> That sounds really nice because we could remove tempGPR...?

Yusuke and Robin has proven to me that this not work.  Ignore me.

>>> Source/JavaScriptCore/jit/AssemblyHelpers.h:967
>>> +        return branch64(NotEqual, tempGPR, TrustedImm32(JSValue::BigInt32Tag));
>> 
>> Can we use branchTest32(Zero, gpr, TrustedImm32(JSValue::BigInt32Tag)) instead?  I think that will generate more efficient code and doesn't need a tempGPR.
> 
> Ditto, we could remove tempGPR use?

Ditto. Does not work.

[Mark Lam]

Comment on attachment 396728 [details]
Patch

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

More comments.

>>>> Source/JavaScriptCore/jit/AssemblyHelpers.h:956
>>>> +        move(gpr, tempGPR);
>>>> +        and64(TrustedImm32(JSValue::BigInt32Tag), tempGPR);
>>>> +        return branch64(Equal, tempGPR, TrustedImm32(JSValue::BigInt32Tag));
>>> 
>>> I think you can use branchTest32(Zero, gpr, TrustedImm32(JSValue::BigInt32Tag)) here.
>> 
>> That sounds really nice because we could remove tempGPR...?
> 
> Yusuke and Robin has proven to me that this not work.  Ignore me.

I think you can use branchTest32(Zero, gpr, TrustedImm32(JSValue::BigInt32Tag)) here.

>>>> Source/JavaScriptCore/jit/AssemblyHelpers.h:967
>>>> +        move(gpr, tempGPR);
>>>> +        and64(TrustedImm32(JSValue::BigInt32Tag), tempGPR);
>>>> +        return branch64(NotEqual, tempGPR, TrustedImm32(JSValue::BigInt32Tag));
>>> 
>>> Can we use branchTest32(Zero, gpr, TrustedImm32(JSValue::BigInt32Tag)) instead?  I think that will generate more efficient code and doesn't need a tempGPR.
>> 
>> Ditto, we could remove tempGPR use?
> 
> Ditto. Does not work.

Can we use branchTest32(Zero, gpr, TrustedImm32(JSValue::BigInt32Tag)) instead?  I think that will generate more efficient code and doesn't need a tempGPR.

> Source/JavaScriptCore/runtime/JSCJSValue.cpp:78
> +        VM& vm = globalObject->vm();
> +        auto scope = DECLARE_THROW_SCOPE(vm);

Please put this at the top of the function.

> Source/JavaScriptCore/runtime/JSCJSValue.cpp:431
> +        // FIXME: we should rather have two cases here: one-character string vs jsNonTrivialString for everything else.

bug url?

> Source/JavaScriptCore/runtime/JSCJSValue.cpp:432
> +        JSString* returnString = JSString::create(vm, bigInt->toString(globalObject, 10).releaseImpl().releaseNonNull());

I see that bigInt->toString() can throw.  So, you'll need an exception check for that before the JSString::create().

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:542
> +    u.asInt64 = ((static_cast<int64_t>(value) << 16) & ~NumberTag) | BigInt32Tag;

You can skip the NumberTag masking if you do this instead:
        u.asInt64 = (static_cast<uint64_t>(value) << 16) | BigInt32Tag;

You can ASSERT that it produces 0s in the NumberTag region.

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:812
> +    VM& vm = globalObject->vm();
> +    auto scope = DECLARE_THROW_SCOPE(vm);

Please put this at the top of this function.  That way all clients of this function will know that this function can potentially throw even if the real value being used won't be one that result in a throw.  The rule is to always declare the ThrowScope at the top of the function (unless there's some special reason why we don't want that ... which should be exceedingly rare).

However, if moving this will cause you grief with missing exception errors and preventing you from landing this patch, then add a FIXME here with a bug to fix this later.

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:834
>      VM& vm = getVM(globalObject);
>      auto scope = DECLARE_THROW_SCOPE(vm);

Ditto.  Put this at the top, or add the FIXME to fix later.

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:856
>      VM& vm = getVM(globalObject);
>      auto scope = DECLARE_THROW_SCOPE(vm);

Ditto.  Move to the top or add FIXME.

> Source/JavaScriptCore/runtime/Operations.cpp:76
> +        Checked<int32_t, RecordOverflow> result = left;

Just use CheckedInt32 instead of Checked<int32_t, RecordOverflow>.

> Source/JavaScriptCore/runtime/Operations.h:247
> +    VM& vm = globalObject->vm();
> +    auto scope = DECLARE_THROW_SCOPE(vm);

For consistency, please put this at the top.

> Source/JavaScriptCore/runtime/Operations.h:260
> +        // FIXME: use something less hacky, e.g. some kind of JSBigInt::compareToInt32

Please file a bug and include the url here fo that we don't forget this.

> Source/JavaScriptCore/runtime/Operations.h:298
> +    // FIXME: have some fast-ish path for a comparison between a small and a large big-int

Please file a bug and add url.

> Source/JavaScriptCore/runtime/Operations.h:484
> -    VM& vm = getVM(globalObject);
> +    VM& vm = globalObject->vm();

Just wondering why this change?  On the other hand, not due to you but why do we have a getVM() function?

> Source/JavaScriptCore/runtime/Operations.h:531
> +        Checked<int32_t, RecordOverflow> result = left;

Just use CheckedInt32.

> Source/JavaScriptCore/runtime/Operations.h:553
> +        Checked<int32_t, RecordOverflow> result = left;

Use CheckedInt32.

> Source/JavaScriptCore/runtime/Operations.h:629
> +        // FIXME: the following next few lines might benefit from being made into a helper function

Bug url?

> Source/JavaScriptCore/runtime/Operations.h:654
> +        // FIXME: the following next few lines might benefit from being made into a helper function

Bug url?

> Source/JavaScriptCore/runtime/Operations.h:716
> +        // FIXME: it might be possible to do something smarter, trying to do the left shift and detecting somehow if it overflowed.

Bug url?

> Source/JavaScriptCore/runtime/Operations.h:730
> +    // FIXME: we should have a case for left is HeapBigInt and right is BigInt32, it seems fairly likely to occur frequently.

Bug url?

> Source/JavaScriptCore/runtime/Operations.h:775
> +    // FIXME: add fast path for BigInt32 / HeapBigInt, and for HeapBigInt / BigInt32
> +    // In many cases these could be made more efficient than heap-allocating the small big-int and going through the generic path.
> +    // But it is quite tricky, not least because HeapBigInt use a sign bit whereas BigInt32 use 2-complement representation.

Bug url?

> Source/JavaScriptCore/runtime/Operations.h:799
> +    // FIXME: currently, for pairs of BigInt32, we unbox them, do the "and" and re-box them.
> +    // We could do it directly on the JSValue.

bug url?

> Source/JavaScriptCore/runtime/Operations.h:810
> +    // FIXME: currently, for pairs of BigInt32, we unbox them, do the "or" and re-box them.
> +    // We could do it directly on the JSValue.

bug url?

> Source/JavaScriptCore/runtime/Operations.h:821
> +    // FIXME: currently, for pairs of BigInt32, we unbox them, do the "xor" and re-box them.
> +    // We could do it directly on the JSValue, and just remember to do an or with 0x12 at the end to restore the tag

bug url?  You can use the same bug url for many or all of the FIXMEs.

[Mark Lam]

Comment on attachment 396728 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:150
>                      auto isNonStringOrBigIntCellConstant = [] (JSValue value) {

I think isNonStringAndNonBigIntCellConstant would be more clear.  Can rename this in a separate patch.

> Source/JavaScriptCore/dfg/DFGOSRExit.h:2
> - * Copyright (C) 2011-2018 Apple Inc. All rights reserved.
> + * Copyright (C) 2011-2020 Apple Inc. All rights reserved.

Revert?

[Robin Morisset]

Comment on attachment 396728 [details]
Patch

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

Thanks a lot for the reviews!

>> JSTests/stress/big-int-right-shift-general.js:5
>> +// Modified by Copyright (C) 2020 Apple.
> 
> I think you should remove the "Modified by" part.  Just say "Copyright (C) 2020 Apple. All rights reserved."  It is understood that the copyright is shared.

Fixed.

>> JSTests/stress/compare-number-and-other.js:75
>>  \ No newline at end of file
> 
> Can you make sure this I not here in the actual source?

I've checked the source and it looks perfectly reasonable. In particular it still has the '}' at the end.. not sure what is going on with diff here.

>> Source/JavaScriptCore/ChangeLog:21
>> +        Note that in this patch we create such small BigInts when parsing BigInt constants, and most operations (e.g. Add or BitOr) produce a BigInt32 if both of their operands are BigInt32,
> 
> I think you mean when "parsing small BigInt constants".  I suggest rephrasing this as "we create BigInt32s when parsing small BigInt constants".  Using "BigInt32" instead of "such small BigInts" also keeps the terminology consistent with the rest of the sentence.

Fixed.

>> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:150
>>                      auto isNonStringOrBigIntCellConstant = [] (JSValue value) {
> 
> I think isNonStringAndNonBigIntCellConstant would be more clear.  Can rename this in a separate patch.

Renamed.

>> Source/JavaScriptCore/dfg/DFGOSRExit.h:2
>> + * Copyright (C) 2011-2020 Apple Inc. All rights reserved.
> 
> Revert?

Fixed.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1402
>> +    GPRReg tempGPR = allocate();
> 
> Use GPRTemporary. `allocate()` and `unlock()` is very low-level concept to implement `GPRTemporary`, `SpeculatedXXXOperand` etc.

Fixed.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1758
>> +        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);

Thanks, I had forgotten about Reuse. Fixed.

>> 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.

Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8752
>> +            speculateHeapBigIntUnknownWhetherCell(m_node->child1());
> 
> Ditto, this is wrong.

Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8768
>> +            speculateHeapBigIntUnknownWhetherCell(m_node->child2());
> 
> Ditto, this is wrong.

Fixed.

>> 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.

Done.

>> 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.

Added.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14679
>> +        LValue right = lowJSValue(node->child2(), ManualOperandSpeculation);
> 
> Why is this `speculate` & `lowJSValue` order swapped?

A mistake. Fixed.

>> 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.

Done.

>> 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.

Done.

>> 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.

Done.

>> Source/JavaScriptCore/runtime/JSCJSValue.cpp:78
>> +        auto scope = DECLARE_THROW_SCOPE(vm);
> 
> Please put this at the top of the function.

Done.

>> Source/JavaScriptCore/runtime/JSCJSValue.cpp:432
>> +        JSString* returnString = JSString::create(vm, bigInt->toString(globalObject, 10).releaseImpl().releaseNonNull());
> 
> I see that bigInt->toString() can throw.  So, you'll need an exception check for that before the JSString::create().

Fixed.

>> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:542
>> +    u.asInt64 = ((static_cast<int64_t>(value) << 16) & ~NumberTag) | BigInt32Tag;
> 
> You can skip the NumberTag masking if you do this instead:
>         u.asInt64 = (static_cast<uint64_t>(value) << 16) | BigInt32Tag;
> 
> You can ASSERT that it produces 0s in the NumberTag region.

Good idea. Done.

>> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:812
>> +    auto scope = DECLARE_THROW_SCOPE(vm);
> 
> Please put this at the top of this function.  That way all clients of this function will know that this function can potentially throw even if the real value being used won't be one that result in a throw.  The rule is to always declare the ThrowScope at the top of the function (unless there's some special reason why we don't want that ... which should be exceedingly rare).
> 
> However, if moving this will cause you grief with missing exception errors and preventing you from landing this patch, then add a FIXME here with a bug to fix this later.

Moved.

>> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:834
>>      auto scope = DECLARE_THROW_SCOPE(vm);
> 
> Ditto.  Put this at the top, or add the FIXME to fix later.

Moved.

>> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:856
>>      auto scope = DECLARE_THROW_SCOPE(vm);
> 
> Ditto.  Move to the top or add FIXME.

Moved.

>> Source/JavaScriptCore/runtime/Operations.cpp:76
>> +        Checked<int32_t, RecordOverflow> result = left;
> 
> Just use CheckedInt32 instead of Checked<int32_t, RecordOverflow>.

Done.

>> Source/JavaScriptCore/runtime/Operations.h:484
>> +    VM& vm = globalObject->vm();
> 
> Just wondering why this change?  On the other hand, not due to you but why do we have a getVM() function?

From a comment on its definition: "Helper function to get VM& from JSGlobalObject* if JSGlobalObject.h is not included."
So we have no reason to bother anywhere that we can just include JSGlobalObject.h

>> Source/JavaScriptCore/runtime/Operations.h:531
>> +        Checked<int32_t, RecordOverflow> result = left;
> 
> Just use CheckedInt32.

Done.

>> Source/JavaScriptCore/runtime/Operations.h:553
>> +        Checked<int32_t, RecordOverflow> result = left;
> 
> Use CheckedInt32.

Done.

[Robin Morisset]

Created attachment 396841 [details]
Patch for landing

Adresses Mark and Yusuke's comments.
Has USE_BIGINT defined as always 0, I will toggle it on in a separate patch (to make it easier to revert if we find it causes regressions).

[Robin Morisset]

Created attachment 396855 [details]
Patch for landing

Fix the windows build failure (just a missing NO_RETURN on emit_op_is_big_int when USE_BIGINT32 is false).

[EWS]

Patch 396855 does not build

[Robin Morisset]

Created attachment 396882 [details]
Patch for landing

Fix the rebase issue that prevented the commit queue from landing the previous one.

[EWS]

Committed r260331: <https://trac.webkit.org/changeset/260331>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396882 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61992701>

[Ross Kirsling]

Hmm, looks like this patch broke the following test262 test:
https://github.com/WebKit/webkit/blob/master/JSTests/test262/test/built-ins/BigInt/constructor-integer.js

[Robin Morisset]

Created attachment 397009 [details]
Patch flipping the flag to true

The previous patch landed with BigInt32 disabled. This patch enables it on 64-bit platforms.

[Robin Morisset]

Comment on attachment 397009 [details]
Patch flipping the flag to true

This already landed actually: https://trac.webkit.org/changeset/260345/webkit

[Saam Barati]

Comment on attachment 396882 [details]
Patch for landing

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

Overall:
- I'm a bit worried about the lack of new testing. We have a lot of new code in the compiler we should be writing hand crafted tests for.
- We need to have new microbenchmark perf tests for this, and what the results of such tests are. This to me is the biggest thing missing from this patch. There is all this work for BigInt32, but we don't even know if it's better.
- It's slightly unsettling the number of FIXMEs you left (by my count, ~80). It's not always obvious what should be in this patch and what should be followups. At this point, I guess they're all followups. But all FIXMEs need linked bugs or should be removed. The ones that actually need their own bugs  are a good set of bugs for next steps on BigInt

> Source/JavaScriptCore/ChangeLog:16
> +        - It cannot be confused with a Double or Integer thanks to the top bits
> +        - It cannot be confused with a pointer to a Cell, thanks to bit 1 which is set to true
> +        - It cannot be confused with a pointer to wasm thanks to bit 0 which is set to false
> +        - It cannot be confused with true/false because bit 2 is set to false
> +        - It cannot be confused for null/undefined because bit 4 is set to true

nice!

> Source/JavaScriptCore/ChangeLog:62
> +        I don't yet have performance numbers for this patch on a BigInt benchmark, I will get such numbers before trying to land it, but I'd like some review in the meantime.

did this ever happen? Seems critically important to actually verify this is faster than what we had before

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:543
> +        // FIXME: this use of binaryUseKind means that we cannot specialize to (for example) a HeapBigInt left-operand and a BigInt32 right-operand.

can you file a bug?

We want all our FIXMEs to link to bugs we might actually fix

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:749
> +            // FIXME: do we really need SpecString here for ValueSub? It seems like we only need it for ValueAdd.

good point. Again, a bug here would be good

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:909
> +        // FIXME: we could do much smarter things for BigInts, see ValueAdd/ValueSub.

file a bug please.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1022
> +        // FIXME: why is this code not shared with ValueSub?

this should be a bug or should be resolved.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1025
> +        else if (node->binaryUseKind() == AnyBigIntUse || node->binaryUseKind() == BigInt32Use)

I think we can do a lot better here. Can we have a mode of math on BigInt32 where we speculate we don't overflow into heap big int?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1614
> +            // FIXME: if the SpeculatedType informs us that we won't have a BigInt32 (or that we won't have a HeapBigInt), then we can transform this node into a IsCellWithType(HeapBigIntType) (or a hypothetical IsBigInt32 node).

again, either do this or file a bug

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1991
> +        // FIXME: why is this check here, and not later (after the type-based replacement by false).
> +        // Saam seems to agree that the two checks could be switched, I'll try that in a separate patch

again, a FIXME with no link

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2012
> +            // FIXME: Is there any case not involving NaN where x === x is not guaranteed to return true?

ditto

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:206
> +                // FIXME: the freezing does not appear useful (since the JSCell is kept alive by vm), but it refuses to compile otherwise.

it's fine to freeze it.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:207
> +                // FIXME: we might optimize inc/dec to a specialized function call instead in that case.

what's "that" case?
Also, no bug

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:222
> +                // FIXME: consider having a special representation for small BigInts instead.

we will definitely want this at some point. I imagine something like what we have for Int52.

Please file a bug

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:225
> +                // FIXME: we might optimize inc/dec to a specialized function call instead in that case.

ditto about what "that" is referring to

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:740
> +            if (Node::shouldSpeculateHeapBigInt(node->child1().node(), node->child1().node())) {

this looks wrong. You want to use child2.node() here, right?

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:3574
> +        // FIXME: what should we do about BigInt32Use and AnyBigIntUse ?
> +        // Also more broadly, what about all of the UseKinds which are not listed in that switch?

This seems like a FIXME more for yourself than one that should be checked in.

Currently, we should nothing for BigInt32 here. But in the future, we'll want to handle it like int 52

For the above Cell cases, we should probably have other cell uses listed there too

> Source/JavaScriptCore/dfg/DFGGraph.cpp:1543
>      if (speculationContains(prediction, SpecBigInt))
> -        return freeze(m_vm.bigIntConstantOne.get());
> +        return freeze(m_vm.heapBigIntConstantOne.get());

This is incomplete.

We should specialize this on BigInt32 vs HeapBigInt if possible. And then resorting to the heap variant if needed

> Source/JavaScriptCore/dfg/DFGNodeFlags.h:57
> +// FIXME: we should have separate flags for HeapBigInt and BigInt32, currently prediction propagation will pessimize things.

needs a bug

Also, how will it pessimize things?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3567
> +    // FIXME: add support for mixed BigInt32 / HeapBigInt

link bug

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3589
> +    speculate(node, child1); // Required for the AnyBigIntUse case

this comment is not needed as this is idiomatic code

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3674
> +        speculate(node, leftChild); // Required for AnyBigIntUse

ditto

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3679
> +        speculate(node, rightChild); // Required for AnyBigIntUse

ditto

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3938
> +    // FIXME: support BigInt32

again, you need a bug for this

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4032
> +        // FIXME: do we really need this extra register, or can we mutate left/right ?

In general, no. The DFG relies on re-using the left/right in the same register for later operations that use these same nodes.

However, you can do a pattern you can look for elsewhere, where you try to "Reuse" one of your children as your result register. This might give resultGPR allocated as one of the inputs, if this node is a kill for one of the inputs.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4044
> +        // FIXME: add some way to eliminate the overflow check

how?
And also, a bug if you really think it's possible

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4047
> +        speculationCheck(Overflow, JSValueRegs(), 0, check);

what's preventing us from doing a forever OSR exit loop here?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4050
> +        jsValueResult(resultGPR, node);

do we not have a bigint32 format result? Seems worth having

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4065
> +        // FIXME: call a more specialized function

link to a bug

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4138
> +        // FIXME: do we really need this extra register, or can we mutate left/right ?

ditto

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4139
> +        // FIXME: also, look into Reuse and other tricks for a more efficient generated code.

yep,
and a bug here too!

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4150
> +        // FIXME: add some way to eliminate the overflow check

ditto: how?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4153
> +        speculationCheck(Overflow, JSValueRegs(), 0, check);

ditto: what's preventing us from exit looping if inputs are always BigInt32s that overflow when operated on together?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4988
> +        // FIXME: do we really need this extra register, or can we mutate left/right ?

ditto

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:4989
> +        // FIXME: also, look into Reuse and other tricks for a more efficient generated code.

ditto

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5003
> +        speculationCheck(Overflow, JSValueRegs(), 0, check);

ditto

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5507
> +    // FIXME: add a fast path for BigInt32. Currently we go through the slow path, because of how ugly the code for Mod gets.

seems like we should be able to share it into a snippet with normal i32 code?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6284
> +    // FIXME: add HeapBigInt case here.
> +    // Not having it means that the compare will not be fused with the branch for this case.

Seems like we should add it!

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6375
> +        if (branchIndexInBlock != UINT_MAX) {
> +            Node* branchNode = m_block->at(branchIndexInBlock);
> +            compilePeepHoleBigInt32Branch(node, branchNode, MacroAssembler::Equal);
> +            use(node->child1());
> +            use(node->child2());
> +            m_indexInBlock = branchIndexInBlock;
> +            m_currentNode = branchNode;
> +            return true;
> +        }

do you have tests for this?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:13333
> +    // FIXME: add a fast path for BigInt32 here (and for typeOf, and for boolify or whatever it is called in this file).

yes, you should do this.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:399
> +        // Instead of doing two branches, we can do a single one, by observing that

nice

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:511
> +    m_jit.xor64(TrustedImm64(invert), resultGPR);

was this wrong before?

Do we have tests?

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

style nit: no need for the outer parens here

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1436
> +            ASSERT(jsValue.isBigInt32());

should be a DFG_ASSERT

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1769
> +    // If we add a DataFormatBool, we should use it here.
> +    m_jit.or32(TrustedImm32(JSValue::ValueFalse), result.gpr());
> +    jsValueResult(result.gpr(), m_currentNode, DataFormatJSBoolean);

why not unblessedBooleanResult?

If you keep it, you should call "boxBoolean" instead of manually doing it

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2154
> +            blessSpeculation(result, Overflow, noValue(), nullptr, m_origin);

ditto about what's preventing OSR exit loop here

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2192
> +            blessSpeculation(result, Overflow, noValue(), nullptr, m_origin);

ditto

The feedback loop that I can't find would be some kind of profiling letting us know if it's OK to speculate this or if we should instead say our return type is "BigInt32 | HeapBigInt" and allow overflow

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2349
> +            // FIXME: This is not supported by the IC yet.

bug

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2629
> +        // FIXME: add a fast path for BigInt32 here

bug

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2710
> +        if (m_node->binaryUseKind() == HeapBigIntUse) {

bug

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2888
> +        // FIXME: maybe add a fast path for BigInt32 here

bug

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3354
> +            // No need to unbox, since the tagging is not affected by bitAnd

bitOr, not bitAnd

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3387
> +            LValue result = m_out.bitOr(resultMissingTag, m_out.constInt64(JSValue::BigInt32Tag));

can we have a helper called "boxBigInt32" in FTLLower and use that here?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3447
> +            m_out.bitAnd(lowInt32(m_node->child2()), m_out.constInt32(31)))); // FIXME: I don't think that the BitAnd is useful, it is included in the semantics of shift in B3

bug

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3454
> +            m_out.bitAnd(lowInt32(m_node->child2()), m_out.constInt32(31)))); // FIXME: I don't think that the BitAnd is useful, it is included in the semantics of shift in B3

ditto

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3460
> +        // FIXME: consider adding a fast path for BigInt32 here.

ditto

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3482
> +            m_out.bitAnd(lowInt32(m_node->child2()), m_out.constInt32(31)))); // FIXME: I don't think that the BitAnd is useful, it is included in the semantics of shift in B3

ditto

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8739
> +            ValueFromBlock resultLeftEqualsRight = m_out.anchor(m_out.booleanTrue);

you could "anchor" this before the branch, and not need a block to jump to, and instead jump to continuation with about equal(left, right) branch

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8752
> +            // FIXME: [ESNext][BigInt] Create specialized version of strict equals for big ints

I feel like we should just inline the comparison

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8938
> +        // FIXME: we can do something much smarter here, see the DFGSpeculativeJIT approach in e.g. SpeculativeJIT::nonSpeculativePeepholeStrictEq

bug

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:17151
> +        // FIXME: we should filter the type passed to isCell to account for the previous test that told us we are definitely not a BigInt32.

should be simple enough to do or have a bug

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:17155
> +        m_out.appendTo(isNotCellCase, isCellCase);
> +        ValueFromBlock returnTrue = m_out.anchor(m_out.booleanTrue);

if you do this anchor before the above branch the above branch can just go to continuation

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:17673
> +    LValue isNotHeapBigIntUnknownWhetherCell(LValue value, SpeculatedType type = SpecFullTop)

This entire implementation below the proven check looks inverted if I'm understand the name.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:17681
> +        ValueFromBlock defaultToFalse = m_out.anchor(m_out.booleanFalse);

should be true, right? If you're not a cell, it's "true" you're not a heap big int

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

inverted, right?

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:760
> +    urshift64(TrustedImm32(16), result);

we need a helper called "unboxBigInt32" and it should be used everywhere instead of emitting this shift manually everywhere. This makes refactoring and reading JIT code like this in the future much easier.

> Source/JavaScriptCore/jit/AssemblyHelpers.h:934
> +    Jump branchIfBigInt32(GPRReg gpr, GPRReg tempGPR, TagRegistersMode mode = HaveTagRegisters)

this probably doesn't need a temp if you change the other branch to just use the masm temp register

> Source/JavaScriptCore/jit/AssemblyHelpers.h:962
> +    Jump branchIfNotBigInt32KnownNotNumber(GPRReg gpr, GPRReg tempGPR)

you're allocating a scratch all over the place for this thing. Why not just use the assembler scratch register? I don't think the other instructions you're using rely on also using that scratch register. Seems much better than what you're currently doing. The only thing you need to ensure is that any patchpoint in FTL that uses these is marked as clobbering that register.

You could also keep this helper, but have one that defaults to using the masm scratch registers.

> Source/JavaScriptCore/jit/AssemblyHelpers.h:971
> +    // FIXME: rename these to make it clear that they require their input to be a cell.

bug

> Source/JavaScriptCore/jit/AssemblyHelpers.h:994
> +    // FIXME: rename these to make it clear that they require their input to be a cell.

bug
alternatively, require caller to wrap input in some type that marks it as a cell, like "class KnownCellGPR { explicit KnownCellGPR(GPRReg r) m_gpr(r) {} operator GPRReg() { return m_gpr; } GPRReg m_gpr; }

> Source/JavaScriptCore/jit/AssemblyHelpers.h:1402
> +    void unboxBigInt32(GPRReg gpr)

you have these! You should use them everywhere :-)

> Source/JavaScriptCore/jit/AssemblyHelpers.h:1407
> +    void boxBigInt32(GPRReg gpr)

ditto

> Source/JavaScriptCore/jit/JITOpcodes.cpp:297
> +    move(TrustedImm64(JSValue::BigInt32Mask), regT1);
> +    and64(regT1, regT0);

this can't be done in one insn?

> Source/JavaScriptCore/jit/JITOpcodes.cpp:621
> +    // Leaving only doubles above or equal 1<<50.

we go to the slow path for doubles?

> Source/JavaScriptCore/jit/JITOpcodes.cpp:637
> +    // FIXME: we could do something more precise: unless there is a BigInt32, we only need to do the slow path if both are strings

bug

> Source/JavaScriptCore/jit/JITOpcodes.cpp:712
> +    move(regT0, regT2);
> +    move(regT1, regT3);
> +    move(TrustedImm64(JSValue::LowestOfHighBits), regT5);
> +    add64(regT5, regT2);
> +    add64(regT5, regT3);
> +    lshift64(TrustedImm32(1), regT5);
> +    or64(regT2, regT3);
> +    addSlowCase(branch64(AboveOrEqual, regT3, regT5));

you do this in a few places. Any way to abstract the code out into a helper somehow?

> Source/JavaScriptCore/jit/JITOperations.cpp:2864
> +// FIXME: it would be better to call those operationValueNegate, since the operand can be a BigInt

bug

> Source/JavaScriptCore/jit/JITOperations.cpp:2880
> +        if (value != INT_MIN)

nit: I think we try to use std::numeric_limits, but I'm not 100% on style guidance here

> Source/JavaScriptCore/jit/JITOperations.cpp:3001
> +    // FIXME: why does this function profile the argument but not the result?

did you figure this out?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:971
> +        # FIXME: the next line jumps to the slow path for BigInt32. We could instead have a dedicated path in here for them.

let's do it, or file a bug

> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:85
> +    // FIXME: heap-allocating all big ints is inneficient, but re-implementing toString for small BigInts is enough work that I'm deferring it to a later patch.

please file a bug on this one

> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:131
> +// FIXME: this function should introduce the right separators for thousands and similar things.

bug

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:549
> +        int32_t value = primValue.bigInt32AsInt32();
> +        if (value != INT_MIN) {

you have this sequence of code in quite a few places. It'd be good to make it a helper for negate

> Source/JavaScriptCore/runtime/JSBigInt.cpp:1896
> +        for (unsigned i = 0; i < lengthLimitForBigInt32 && p < length ; ++i, ++p) {

style nit: no space before ";"

> Source/JavaScriptCore/runtime/JSBigInt.cpp:1918
> +                    digit *= -1;

this looks wrong if it's INT_MIN. Do we have a test?

> Source/JavaScriptCore/runtime/JSCJSValue.h:454
> +    static constexpr int64_t LowestOfHighBits = 1ULL << 49;

nit: this name doesn't seem fully expressive. Maybe "LowestOfNumberTagBits" or something similar. "HighBits" is kinda ambiguous.

It's also not the end of the world if we can't think of a better name since these asserts below give the idea quickly.

> Source/JavaScriptCore/runtime/JSCJSValue.h:456
> +    static_assert(!((LowestOfHighBits>>1) & NumberTag));

style:  need a space between ">>" and lhs/rhs

Also, should we add another static cast that this + NumberTag == 0? Since we're relying on the overflow here (with whatever casting to unsigned needed to make it not UB)

> Tools/Scripts/run-jsc-stress-tests:711
> +def runBigIntEnabledNoJIT(*optionalTestSpecificOptions)
> +    run("big-int-enabled-nojit", "--useBigInt=true", "--useJIT=false", *optionalTestSpecificOptions)
> +end
> +
> +def runBigIntEnabledBaseline(*optionalTestSpecificOptions)
> +    run("big-int-enabled-baseline", "--useBigInt=true", "--useDFGJIT=0", *optionalTestSpecificOptions)
> +end

do we really need these now that big int is enabled by default?

[Michael Catanzaro]

No follow-up to Saam's review?

It introduced a couple build warnings we should fix:

[151/1959] Building CXX object Source/...riptCore.dir/ftl/FTLLowerDFGToB3.cpp.o
/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp: In member function ‘void JSC::FTL::{anonymous}::LowerDFGToB3::compileValueBitNot()’:
/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3346:49: warning: comparison of integer expressions of different signedness: ‘const int64_t’ {aka ‘const long int’} and ‘long unsigned int’ [-Wsign-compare]
 3346 |             static_assert(JSValue::BigInt32Mask == 0xfffe000000000012);
      |                           ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~

[152/1959] Building CXX object Source/...ptCore.dir/dfg/DFGSpeculativeJIT.cpp.o
/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp: In member function ‘void JSC::DFG::SpeculativeJIT::compileValueBitNot(JSC::DFG::Node*)’:
/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3600:45: warning: comparison of integer expressions of different signedness: ‘const int64_t’ {aka ‘const long int’} and ‘long unsigned int’ [-Wsign-compare]
 3600 |         static_assert(JSValue::BigInt32Mask == 0xfffe000000000012);
      |                       ~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~

0xfffe000000000012 is too big for int, unsigned int, or long int, so it uses long unsigned int. Then problem is JSValue::BigInt32Mask is a *negative-valued signed integer,* which is probably not intended? To fix these asserts without modifying JSValue::BigInt32Mask, we'd have to do this:

static_assert(JSValue::BigInt32Mask == static_cast<int64_t>(0xfffe000000000012));

I can submit a patch for that, but maybe revisiting the types of the masks would be better since masks are generally best when unsigned. (Of course, changing the types of the masks will likely result in a bunch of new warnings. :)

[Michael Catanzaro]

Reported bug #211783.