[WASM-References] Add support for Anyref in parameters and return types, Ref.null and Ref.is_null for Anyref values.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Justin Michaud]

Add support for Anyref in parameters and return types of Wasm functions, Ref.null and Ref.is_null for Anyref values.

Support for funcref (formerly anyfunc) is out of scope for this bug.

[Justin Michaud]

Created attachment 370091 [details]
Patch

[EWS Watchlist]

This patch modifies one of the wasm.json files. Please ensure that any changes in one have been mirrored to the other. You can find the wasm.json files at "Source/JavaScriptCore/wasm/wasm.json" and "JSTests/wasm/wasm.json".

[Yusuke Suzuki]

Comment on attachment 370091 [details]
Patch

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

r=me

> Source/JavaScriptCore/wasm/WasmValidate.cpp:177
> +    WASM_VALIDATOR_FAIL_IF(Type::Anyref != value, "ref.is_null to type ", value, " expected ", Type::Anyref);

You can define a helper function like, `isReferenceType()` instead of directly checking AnyRef, and in the future, you can extend this function to accept funcref too.

> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:261
> +        isNullref.link(&jit);

Discussed with Justin, at this point, we can just use JSValue representation directly.
We could change this after revisiting thread proposal.

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:262
> +                        if (!buffer[argNum])
> +                            arg = jsNull();
> +                        else
> +                            arg = JSValue::decode(buffer[argNum]);

Ditto.

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:300
> +                    if (result.isNull())
> +                        realResult = 0;
> +                    else
> +                        realResult = JSValue::encode(result);

Ditto.

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:427
> +                // If 0, this is nullref so it becomes null. Otherwise, it is already a JSValue.
> +                auto isNullref = jit.branchIfNotEmpty(gprReg);
> +                jit.moveTrustedValue(jsNull(), JSValueRegs { gprReg });
> +                isNullref.link(&jit);

Ditto.

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:598
> +        isNullref.link(&jit);

Ditto.

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:90
> +        case Wasm::Anyref:
> +            // All JSValues are valid, but null becomes 0.
> +            if (arg.isNull())
> +                arg = JSValue::decode(0);

Ditto.

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:465
> +        isNullref.link(&jit);

Ditto.

[Saam Barati]

Comment on attachment 370091 [details]
Patch

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

Everything looks great! r- because I think I found a bug in the stub that we use to call from JS to Wasm. Let's add a test for that and get this landed

> Source/JavaScriptCore/ChangeLog:7
> +        Add support for Anyref in parameters and return types of Wasm functions.
> +        Add Ref.null and Ref.is_null for Anyref values. Support for these functions with funcrefs is out of scope.

style nit: this should go below "Reviewed by ..."

Also, you should talk about how you implemented this here. What from JS maps to null in Wasm? etc

> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:258
> +        // If 0, this is nullref so it becomes null. Otherwise, it is already a JSValue.

not sure this comment is adding much

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:183
> +            case Anyref: {

Can this just be part of the I32 case above, and the call to zeroExtend can branch on Anyref vs I32?

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:424
> +                // If 0, this is nullref so it becomes null. Otherwise, it is already a JSValue.

comment isn't adding much that the code doesn't tell you.

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:425
> +                auto isNullref = jit.branchIfNotEmpty(gprReg);

this variable is named wrong. Should be something like "not null ref"

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:595
> +        // Any JSValue, Ref.func or Ref.null is valid here, and it is impossible to create any other kind of ref from within wasm.
> +        // Hence, there are no exception checks.
> +
> +        // If null, this is nullref so it becomes 0. Otherwise, it is still a JSValue.

same about comment not being helpful.

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:596
> +        auto isNullref = jit.branchIfNotNull(GPRInfo::returnValueGPR);

nit: Wrong name. This is JSValue null, so should be named something like "isNull"

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:88
> +            // All JSValues are valid, but null becomes 0.

ditto about comment

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:461
> +    case Wasm::Anyref: {

You're not handling this as a parameter in this JS to Wasm stub. I think that's a bug. Please add a test.

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:462
> +        // If 0, this is nullref so it becomes null. Otherwise, it is already a JSValue.

ditto

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:463
> +        auto isNullref = jit.branchIfNotEmpty(GPRInfo::returnValueGPR);

nit: name should be inverted to "not null ref"

[Saam Barati]

Comment on attachment 370091 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:295
> +    case RefNull: {
> +        m_expressionStack.append(m_context.addConstant(Anyref, 0));
> +        return { };
> +    }
> +
> +    case RefIsNull: {
> +        ExpressionType result, value;
> +        WASM_TRY_POP_EXPRESSION_STACK_INTO(value, "ref.is_null");
> +        WASM_TRY_ADD_TO_CONTEXT(addRefIsNull(value, result));
> +        m_expressionStack.append(result);
> +        return { };
> +    }

I previously said let's not add a runtime option, but I'm going back on that. Let's add a runtime option that we just check here inside the parser. So we can pretend we don't parse this when the option is false. We can even default this option to on for now, but it will give us an easy switch to flip if we want to turn this feature off on some branch in the future (we may want to do that if it's not complete yet).

[Saam Barati]

Comment on attachment 370091 [details]
Patch

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

> JSTests/wasm/wasm.json:20
> +    "block_type": ["i32", "i64", "f32", "f64", "void", "anyref"],

Can we add a test where we have a block with this result type? Maybe like an if or something

[Saam Barati]

Comment on attachment 370091 [details]
Patch

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

>> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:461
>> +    case Wasm::Anyref: {
> 
> You're not handling this as a parameter in this JS to Wasm stub. I think that's a bug. Please add a test.

You can test this by calling some Wasm stuff in a loop to 1000 or 10000 in JS.

[Saam Barati]

Comment on attachment 370091 [details]
Patch

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

>> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:261
>> +        isNullref.link(&jit);
> 
> Discussed with Justin, at this point, we can just use JSValue representation directly.
> We could change this after revisiting thread proposal.

Good point. We'll just need to keep in mind we may need to branch on jsNull in some places we store zero to mean null (I think Table IIRC) when we implement that functionality in the future.

[Justin Michaud]

Created attachment 370157 [details]
Patch

[Justin Michaud]

Created attachment 370159 [details]
Patch

[Keith Miller]

Comment on attachment 370159 [details]
Patch

r=me.

[WebKit Commit Bot]

Comment on attachment 370159 [details]
Patch

Clearing flags on attachment: 370159

Committed r245496: <https://trac.webkit.org/changeset/245496>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

Comment on attachment 370159 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:941
> +    append(Move, Arg::bigImm(JSValue::encode(jsNull())), tmp);
> +    append(Compare64, Arg::relCond(MacroAssembler::Equal), value, tmp, result);

I think this compare can be against an immediate in the instruction instead of moving jsNull to a register. jsNull() is just "2"