1512711 - [WIP] Implement i64<>JavaScript’s BigInt conversions proposal

Status: RESOLVED DUPLICATE of bug 1511958

(Core :: JavaScript: WebAssembly, enhancement, P3)

Description

[Sven Sauleau]

Attachment: basic_support.patch

Doesn't support the JIT currently.

Comment 1

[Andy Wingo [:wingo]]

Comment on attachment 9030008 [details] [diff] [review]
basic_support.patch

Review of attachment 9030008 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good, some changes needed.  Sven what's your timeline on this?  If you address comments, please submit to phabricator instead of as a patch.

::: js/public/Value.h
@@ +63,5 @@
>  #ifdef ENABLE_BIGINT
>    JSVAL_TYPE_BIGINT = 0x09,
>  #endif
>    JSVAL_TYPE_OBJECT = 0x0c,
> +  JSVAL_TYPE_INT64 = 0xd,

This isn't needed; (u)int64_t values should be represented as bigints.

::: js/src/wasm/WasmBuiltins.cpp
@@ +288,1 @@
>    JSContext* cx = TlsContext.get();

Here we should assert that the current context has disabled bigints, and only emit calls here if the context has disabled bigints.

::: js/src/wasm/WasmInstance.cpp
@@ +112,5 @@
>    if (!args.init(cx, argc)) {
>      return false;
>    }
>  
> +#ifndef ENABLE_WASM_BIGINT

Likewise here, error only if bigint not available, and needs support below to unpack the arg.

@@ +1152,5 @@
>    Tier tier = code().bestTier();
>  
>    const FuncExport& func = metadata(tier).lookupFuncExport(funcIndex);
>  
> +#ifndef ENABLE_WASM_BIGINT

Likewise for dynamic check.

@@ +1261,5 @@
>      case ExprType::I32:
>        args.rval().set(Int32Value(*(int32_t*)retAddr));
>        break;
>      case ExprType::I64:
> +#ifdef ENABLE_WASM_BIGINT

needs dynamic check

::: js/src/wasm/WasmJS.cpp
@@ +156,5 @@
>        }
>        return true;
>      }
> +    case ValType::I64: {
> +#ifdef ENABLE_WASM_BIGINT

needs dynamic check

@@ +161,5 @@
> +      BigInt* bigint = ToBigInt(cx, v);
> +      if (!bigint) {
> +        return false;
> +      }
> +      val.set(Val(uint64_t(BigInt::toInt64(bigint))));

use toUInt64

@@ +189,5 @@
>          return NullValue();
>        }
>        return ObjectValue(*(JSObject*)val.ptr());
> +    case ValType::I64: {
> +#ifdef ENABLE_WASM_BIGINT

needs dynamic check

@@ +333,5 @@
>            obj->val(&val);
>          } else {
>            if (IsNumberType(global.type())) {
> +#ifdef ENABLE_WASM_BIGINT
> +            if (global.type() != ValType::I64 && !v.isNumber()) {

This isn't very clear to me; how about replacing with

```
       } else {
         if (global.type() == ValType::I64) {
           if (!v.isBigInt()) {
             return ThrowBadImportType(cx, import.field.get(), "BigInt");
           }
         } else if (IsNumberType(global.type()) {
           if (!v.isNumber()) {
             return ThrowBadImportType(cx, import.field.get(), "Number");
           }
         }
```

@@ +349,5 @@
>                                          "Object-or-null");
>              }
>            }
>  
> +#ifndef ENABLE_WASM_BIGINT

replace with dynamic check

@@ +2552,5 @@
>      case ValType::AnyRef:
>        args.rval().set(args.thisv().toObject().as<WasmGlobalObject>().value(cx));
>        return true;
>      case ValType::I64:
> +#ifdef ENABLE_WASM_BIGINT

dynamic check

@@ +2588,5 @@
>                               JSMSG_WASM_GLOBAL_IMMUTABLE);
>      return false;
>    }
>  
> +#ifndef ENABLE_WASM_BIGINT

dynamic check

@@ +2622,5 @@
>        }
>        break;
>      }
>      case ValType::I64:
> +#ifdef ENABLE_WASM_BIGINT

Here we need to use BigInt::createFromInt64(cx, val.get().signed_u64()).

::: js/src/wasm/WasmStubs.cpp
@@ +571,5 @@
>    masm.reserveStack(frameSize);
>  
>    GenerateJitEntryLoadTls(masm, frameSize);
>  
> +#ifndef ENABLE_WASM_BIGINT

Only emit the call if the context has disabled bigints.  (Do we have context access here?)

@@ +641,5 @@
>            masm.storeValue(JSVAL_TYPE_INT32, scratchG, jitArgAddr);
>          }
>          break;
>        }
> +#ifdef ENABLE_WASM_BIGINT

Here you want to callVM to BigInt::createFromInt64.

@@ +727,5 @@
>            masm.storePtr(target, Address(sp, iter->offsetFromArgBase()));
>          }
>          break;
>        }
> +#ifdef ENABLE_WASM_BIGINT

I think here, callVM to BigInt::asInt64.

@@ +813,5 @@
>      case ExprType::AnyRef:
>        MOZ_CRASH("return anyref in jitentry NYI");
>        break;
>      case ExprType::I64:
> +#ifdef ENABLE_WASM_BIGINT

BigInt::createFromUint64

@@ +1033,5 @@
>        masm.canonicalizeDouble(ReturnDoubleReg);
>        break;
>      case wasm::ExprType::Ref:
>      case wasm::ExprType::AnyRef:
> +#ifdef ENABLE_WASM_BIGINT

I think here we need to expect that Ion will handle the unboxing, ideally via some MTruncateBigInt64 / MTruncateBigUInt64 or so that can be optimized away, but otherwise manually.  So this is the right thing.

Comment 2

[Lars T Hansen [:lth]]

As I understand it, this bug is essentially a prototype for bug 1511958. Should we close this one as a duplicate of that?

Comment 3

[Asumu Takikawa]

I agree, I think it's ok to close this bug as a duplicate. I've changed the status now.