BigInt math runtime shouldn't convert BigInt32 input operands to a heap cell when doing math

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

We could define an abstract interface and use that in the math runtime instead.

[Saam Barati]

Created attachment 397536 [details]
WIP

it begins

[Keith Miller]

The title of this bug seems a bit ambitious in the abstract... 🤔

[Saam Barati]

(In reply to Keith Miller from comment #2)
> The title of this bug seems a bit ambitious in the abstract... 🤔

How so?

[Saam Barati]

Created attachment 397743 [details]
WIP

[Saam Barati]

Created attachment 397771 [details]
WIP

it compiles

[Saam Barati]

Created attachment 397782 [details]
patch

[Saam Barati]

Created attachment 397785 [details]
patch

[Saam Barati]

Created attachment 397786 [details]
patch

[Yusuke Suzuki]

Comment on attachment 397785 [details]
patch

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

r=me

> Source/JavaScriptCore/runtime/Operations.cpp:79
> +    RELEASE_AND_RETURN(scope, arithmeticBinaryOp(globalObject, p1, p2, doubleOp, bigIntOp, bigIntOp, bigIntOp, bigIntOp, "Invalid mix of BigInt and other type in addition."_s));

See the comment below.

> Source/JavaScriptCore/runtime/Operations.h:531
> +ALWAYS_INLINE JSValue arithmeticBinaryOp(JSGlobalObject* globalObject, JSValue v1, JSValue v2, DoubleOperation&& doubleOp, JSValue (*bigInt32)(JSGlobalObject*, int32_t, int32_t), JSValue (*mixedBigInt1)(JSGlobalObject*, JSBigInt*, int32_t), JSValue (*mixedBigInt2)(JSGlobalObject*, int32_t, JSBigInt*), JSValue (*heapBigInt)(JSGlobalObject*, JSBigInt*, JSBigInt*), const char* errorMessage)

Why not taking auto-parameterized lambda as a parameterized type-ed value and call it for all the BigInt cases?

tempalte<typename DoubleOperation, typename BigIntOperation>
ALWAYS_INLINE JSValue arithmeticBinaryOp(JSGlobalObject* globalObject, JSValue v1, JSValue v2, DoubleOperation&& doubleOp, BigIntOperation&& bigIntOp, const char* errorMessage)

And always call bigIntOp for BigInts.

> Source/JavaScriptCore/runtime/Operations.h:547
> +            RELEASE_AND_RETURN(scope, bigInt32(globalObject, leftNumeric.bigInt32AsInt32(), rightNumeric.bigInt32AsInt32()));

So, then, you can write it as `ELEASE_AND_RETURN(scope, bigIntOp(globalObject, leftNumeric.bigInt32AsInt32(), rightNumeric.bigInt32AsInt32()));`

> Source/JavaScriptCore/runtime/Operations.h:549
> +            RELEASE_AND_RETURN(scope, mixedBigInt2(globalObject, leftNumeric.bigInt32AsInt32(), rightNumeric.asHeapBigInt()));

`RELEASE_AND_RETURN(scope, bigIntOp(globalObject, leftNumeric.bigInt32AsInt32(), rightNumeric.asHeapBigInt()));`

> Source/JavaScriptCore/runtime/Operations.h:552
> +            RELEASE_AND_RETURN(scope, mixedBigInt1(globalObject, leftNumeric.asHeapBigInt(), rightNumeric.bigInt32AsInt32()));

RELEASE_AND_RETURN(scope, bigIntOp(globalObject, leftNumeric.asHeapBigInt(), rightNumeric.bigInt32AsInt32()));

> Source/JavaScriptCore/runtime/Operations.h:554
> +            RELEASE_AND_RETURN(scope, heapBigInt(globalObject, leftNumeric.asHeapBigInt(), rightNumeric.asHeapBigInt()));

RELEASE_AND_RETURN(scope, bigIntOp(globalObject, leftNumeric.asHeapBigInt(), rightNumeric.asHeapBigInt()));

> Source/JavaScriptCore/runtime/Operations.h:559
> +        RELEASE_AND_RETURN(scope, heapBigInt(globalObject, leftNumeric.asHeapBigInt(), rightNumeric.asHeapBigInt()));

RELEASE_AND_RETURN(scope, bigIntOp(globalObject, leftNumeric.asHeapBigInt(), rightNumeric.asHeapBigInt()));

> Source/JavaScriptCore/runtime/Operations.h:575
> +    return arithmeticBinaryOp(globalObject, v1, v2, doubleOp, bigIntOp, bigIntOp, bigIntOp, bigIntOp, "Invalid mix of BigInt and other type in subtraction."_s);

In that case, we do not need to pass 4 bigIntOp here. You can just call it,

return arithmeticBinaryOp(globalObject, v1, v2, doubleOp, bigIntOp, "Invalid mix of BigInt and other type in subtraction."_s);

> Source/JavaScriptCore/runtime/Operations.h:588
> +    return arithmeticBinaryOp(globalObject, v1, v2, doubleOp, bigIntOp, bigIntOp, bigIntOp, bigIntOp, "Invalid mix of BigInt and other type in multiplication."_s);

Ditto.

> Source/JavaScriptCore/runtime/Operations.h:601
> +    return arithmeticBinaryOp(globalObject, v1, v2, doubleOp, bigIntOp, bigIntOp, bigIntOp, bigIntOp, "Invalid mix of BigInt and other type in division."_s);

Ditto.

> Source/JavaScriptCore/runtime/Operations.h:614
> +    return arithmeticBinaryOp(globalObject, v1, v2, doubleOp, bigIntOp, bigIntOp, bigIntOp, bigIntOp, "Invalid mix of BigInt and other type in remainder."_s);

Ditto.

> Source/JavaScriptCore/runtime/Operations.h:627
> +    return arithmeticBinaryOp(globalObject, v1, v2, doubleOp, bigIntOp, bigIntOp, bigIntOp, bigIntOp, "Invalid mix of BigInt and other type in exponentiation."_s);

Ditto.

> Source/JavaScriptCore/runtime/Operations.h:774
> -    Optional<uint32_t> leftUint32 = left.toUInt32AfterToNumeric(globalObject);
> +    auto leftNumeric = left.toBigIntOrInt32(globalObject);
>      RETURN_IF_EXCEPTION(scope, { });
> -    Optional<uint32_t> rightUint32 = right.toUInt32AfterToNumeric(globalObject);
> +    auto rightNumeric = right.toBigIntOrInt32(globalObject);
>      RETURN_IF_EXCEPTION(scope, { });
>  
> -    if (UNLIKELY(!leftUint32 || !rightUint32)) {
> +    if (UNLIKELY(!leftNumeric.isInt32() || !rightNumeric.isInt32())) {
>          throwTypeError(globalObject, scope, "BigInt does not support >>> operator"_s);
>          return { };
>      }
>  
> -    return jsNumber(static_cast<int32_t>(leftUint32.value() >> (rightUint32.value() & 31)));
> +    uint32_t leftUint32 = static_cast<uint32_t>(leftNumeric.asInt32());
> +    uint32_t rightUint32 = static_cast<uint32_t>(rightNumeric.asInt32());
> +
> +    return jsNumber(static_cast<int32_t>(leftUint32 >> (rightUint32 & 31)));

Why is it changed?

> Source/JavaScriptCore/runtime/Operations.h:778
> +ALWAYS_INLINE JSValue bitwiseBinaryOp(JSGlobalObject* globalObject, JSValue v1, JSValue v2, Int32Operation&& int32Op, JSValue (*bigInt32)(JSGlobalObject*, int32_t, int32_t), JSValue (*mixedBigInt1)(JSGlobalObject*, JSBigInt*, int32_t), JSValue (*mixedBigInt2)(JSGlobalObject*, int32_t, JSBigInt*), JSValue (*heapBigInt)(JSGlobalObject*, JSBigInt*, JSBigInt*), const char* errorMessage)

Ditto.

> Source/JavaScriptCore/runtime/Operations.h:824
> +    return bitwiseBinaryOp(globalObject, v1, v2, int32Op, bigIntOp, bigIntOp, bigIntOp, bigIntOp, "Invalid mix of BigInt and other type in bitwise 'and' operation."_s);

Ditto.

> Source/JavaScriptCore/runtime/Operations.h:840
> +    return bitwiseBinaryOp(globalObject, v1, v2, int32Op, bigIntOp, bigIntOp, bigIntOp, bigIntOp, "Invalid mix of BigInt and other type in bitwise 'or' operation."_s);

Ditto.

> Source/JavaScriptCore/runtime/Operations.h:855
> +    return bitwiseBinaryOp(globalObject, v1, v2, int32Op, bigIntOp, bigIntOp, bigIntOp, bigIntOp, "Invalid mix of BigInt and other type in bitwise 'xor' operation."_s);

Ditto.

[Yusuke Suzuki]

Comment on attachment 397786 [details]
patch

r=me if EWS is happy :)

[Saam Barati]

Comment on attachment 397785 [details]
patch

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

>> Source/JavaScriptCore/runtime/Operations.h:774
>> +    return jsNumber(static_cast<int32_t>(leftUint32 >> (rightUint32 & 31)));
> 
> Why is it changed?

I think this is a bad rebase. Will revert

[Saam Barati]

landed in:
https://trac.webkit.org/changeset/260809/webkit

[Radar WebKit Bug Importer]

<rdar://problem/62490218>

[Yusuke Suzuki]

Committed r260876: <https://trac.webkit.org/changeset/260876>

[Yusuke Suzuki]

Committed r260877: <https://trac.webkit.org/changeset/260877>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 211165

[Saam Barati]

This shouldn't have been re-opened.

Build fix in:
https://trac.webkit.org/changeset/260880/webkit