Bug 197969 - [WASM-References] Add support for Anyref in parameters and return types, Ref.null and Ref.is_null for Anyref values.
Summary: [WASM-References] Add support for Anyref in parameters and return types, Ref....
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Michaud
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-05-16 16:25 PDT by Justin Michaud
Modified: 2019-05-27 23:56 PDT (History)
8 users (show)

See Also:


Attachments
Patch (34.13 KB, patch)
2019-05-16 16:43 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (40.23 KB, patch)
2019-05-17 15:09 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (40.22 KB, patch)
2019-05-17 15:16 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Michaud 2019-05-16 16:25:51 PDT
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.
Comment 1 Justin Michaud 2019-05-16 16:43:00 PDT
Created attachment 370091 [details]
Patch
Comment 2 Build Bot 2019-05-16 16:44:34 PDT
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".
Comment 3 Yusuke Suzuki 2019-05-16 17:57:36 PDT
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.
Comment 4 Saam Barati 2019-05-16 18:01:03 PDT
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"
Comment 5 Saam Barati 2019-05-16 18:03:00 PDT
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).
Comment 6 Saam Barati 2019-05-16 18:05:28 PDT
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
Comment 7 Saam Barati 2019-05-16 18:07:30 PDT
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.
Comment 8 Saam Barati 2019-05-16 18:09:33 PDT
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.
Comment 9 Justin Michaud 2019-05-17 15:09:26 PDT
Created attachment 370157 [details]
Patch
Comment 10 Justin Michaud 2019-05-17 15:16:44 PDT
Created attachment 370159 [details]
Patch
Comment 11 Keith Miller 2019-05-17 16:10:59 PDT
Comment on attachment 370159 [details]
Patch

r=me.
Comment 12 WebKit Commit Bot 2019-05-17 21:58:42 PDT
Comment on attachment 370159 [details]
Patch

Clearing flags on attachment: 370159

Committed r245496: <https://trac.webkit.org/changeset/245496>
Comment 13 WebKit Commit Bot 2019-05-17 21:58:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Saam Barati 2019-05-27 23:56:47 PDT
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"