Bug 202250 – Add support for the Wasm multi-value proposal

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

Add support for the Wasm multi-value proposal

[Keith Miller]

Created attachment 379607 [details]
Patch

[Keith Miller]

Created attachment 379610 [details]
Patch

[Keith Miller]

Created attachment 379614 [details]
Just code changes

[Keith Miller]

Created attachment 379615 [details]
Patch

[Keith Miller]

GTK fix should be easy and I have it locally (probably). Will upload with any other fixes or before landing.

[Saam Barati]

Comment on attachment 379615 [details]
Patch

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

> JSTests/ChangeLog:13
> +        This patch adds a new way to run stress tests via the .wat text
> +        format. By attaching an asm.js compiled version of the wabt tool
> +        we can easily create wat files programatically and convert them
> +        into a wasm blob to compile. To make this easy there is a
> +        wabt-wrapper.js module file that exports two useful functions that
> +        correspond to WebAssembly.compile and WebAssembly.instantiate.

can you document here how we can update the wabt tool in the future?

[Saam Barati]

Comment on attachment 379614 [details]
Just code changes

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

will continue to review later tonight or tomorrow, but I'll post my comments so far.

> Source/JavaScriptCore/ChangeLog:18
> +        way by using the fact that the old value type trivial signature

this sentence needs some grammar editing

> Source/JavaScriptCore/ChangeLog:35
> +        calling conventions code and moves that logic into the Air IR

conventions => convention

> Source/JavaScriptCore/b3/B3StackmapGenerationParams.h:99
> +    // These are provided for convenience; they means that you don't have to capture them if you don't want to.

"they means" => "they mean"

> Source/JavaScriptCore/jit/AssemblyHelpers.h:92
> +    void store64FromReg(Reg src, Address dst)
> +    {
> +        if (src.isFPR())
> +            storeDouble(src.fpr(), dst);
> +        else
> +            store64(src.gpr(), dst);
> +    }
> +
> +    void store32FromReg(Reg src, Address dst)
> +    {
> +        if (src.isFPR())
> +            storeFloat(src.fpr(), dst);
> +        else
> +            store32(src.gpr(), dst);
> +    }

do we really need to names these "fromReg"?

> Source/JavaScriptCore/runtime/JSCConfig.h:43
> +constexpr size_t ConfigSizeToProtect = 32 * KB;

:(

Do we really have this many options?

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:424
> +        emitPatchpoint(basicBlock, patch, Vector<Tmp, 1> { result }, Vector<ConstrainedTmp, sizeof...(Args)>::from(theArgs...));

nit: I wonder if just "{ result }" would work

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:432
> +    template <typename ResultTmpType, size_t inlineSize>

why is ResultTmpType a template type? It's not static?

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:496
> +                // If the patch is a terminal this is a return.

to be pedantic: "a return" => "a multivalve return"

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:238
> +//    PartialResult WARN_UNUSED_RETURN splitStack(BlockSignature, Stack& currentStack, Stack& newStack);

please remove

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1172
> +static B3IRGenerator::Stack splitStack(BlockSignature signature, B3IRGenerator::Stack& stack)

can this be written in such a way that it's agnostic of Air vs B3. Essentially identical code exists in the Air IR generator too

[Saam Barati]

Comment on attachment 379614 [details]
Just code changes

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

so far still LGTM. I mostly have nits but I think I've found one bug. I will continue to review later, but leaving comments here now so you can address them

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2033
> +    CallInformation locations = wasmCallingConvention().callLocationsFor(signature);

nit: name of this function and variable seems weird. Why not call the API "callInformationFor" instead of "callLocationsFor"?

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2040
> +    RELEASE_ASSERT(!newSize.hasOverflowed());
> +
> +    patchArgs.grow(newSize.unsafeGet());

nit: can't you just call the "get" that crashes if overflow instead of the assert?

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2953
> +    emitPatchpoint(m_currentBlock, patchpoint, Vector<Tmp, 1> { result }, WTFMove(args));

same comment as earlier. I wonder if "{ result }" will Just Work here

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1443
> +    ASSERT(returnValues.size() >= wasmCC.results.size());

nit: release assert

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1446
> +        auto rep = wasmCC.results[i];

nit: type name or a longer variable name here is nicer IMO

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1546
> +    for (unsigned i = 0; i < args.size(); ++i)
> +    constrainedArguments.append(B3::ConstrainedValue(args[i], wasmCC.params[i]));

indent is wrong

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1578
> +            const Vector<B3::Type>& tuple = m_proc.tupleForType(returnType);
> +            for (unsigned i = 0; i < signature.returnCount(); ++i)

can we assert that these numbers are the same size? Should they be?

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1845
> +    switch (returnType.kind()) {
> +    case B3::Void: {
> +        break;
> +    }
> +    case B3::Tuple: {
> +        const Vector<B3::Type>& tuple = m_proc.tupleForType(returnType);
> +        for (unsigned i = 0; i < signature.returnCount(); ++i)
> +            results.append(m_currentBlock->appendNew<ExtractValue>(m_proc, origin(), tuple[i], callResult, i));
> +        break;
> +    }
> +    default: {
> +        results.append(callResult);
> +        break;
> +    }
> +    }

why not abstract "fillResults" lambda above?

> Source/JavaScriptCore/wasm/WasmCallingConvention.cpp:78
> +        WTF::storeStoreFence();

why? Seems unneeded. Seems like std::once would be wrong if this was needed

> Source/JavaScriptCore/wasm/WasmCallingConvention.h:39
>  #include "B3StackmapGenerationParams.h"

I'm a very huge fan of the changes in this file and how it's used by the rest of the implementation. Thanks for doing it.

> Source/JavaScriptCore/wasm/WasmCallingConvention.h:91
> +    enum CallMode : bool {

enum class?

> Source/JavaScriptCore/wasm/WasmCallingConvention.h:93
> +        Caller = true,
> +        Callee = false,

do we actually rely on the values here?

> Source/JavaScriptCore/wasm/WasmCallingConvention.h:134
> +        if (forCaller)

I prefer if this just compares to enum name explicitly. Is there a reason not to? (I think you can then just make enum class and remove default values for enum names)

> Source/JavaScriptCore/wasm/WasmCallingConvention.h:188
> +    enum CallMode : bool {
> +        Caller = true,
> +        Callee = false,
> +    };

same comment as before. Can we just make this an enum class without default values which lives outside these two classes?

> Source/JavaScriptCore/wasm/WasmParser.h:284
> +    WASM_PARSER_FAIL_IF(!Options::useWebAssemblyMultiValues(), "Type table indicies for block signatures are not supported yet");

"indicies" => "indices"

> Source/JavaScriptCore/wasm/WasmParser.h:287
> +    WASM_PARSER_FAIL_IF(!parseVarInt64(index), "Block-like instruction doesn't return value type but can't decode length");

nit: this error message could be made slightly more clear IMO

> Source/JavaScriptCore/wasm/WasmSignature.cpp:164
> +    static void translate(SignatureHash& entry, const ParameterTypes& params, unsigned)
> +    {
> +        RefPtr<Signature> signature = Signature::tryCreate(params.returnTypes.size(), params.argumentTypes.size());
> +        RELEASE_ASSERT(signature);
> +
> +        for (unsigned i = 0; i < params.returnTypes.size(); ++i)
> +            signature->getReturnType(i) = params.returnTypes[i];
> +
> +        for (unsigned i = 0; i < params.argumentTypes.size(); ++i)
> +            signature->getArgument(i) = params.argumentTypes[i];
> +
> +        entry.key = WTFMove(signature);
> +    }

what's this used for?

> Source/JavaScriptCore/wasm/WasmValidate.cpp:310
> +static Validate::Stack splitStack(BlockSignature signature, Validate::Stack& stack)

this is the third place you do it. Let's just abstract this.

> Source/JavaScriptCore/wasm/WasmValidate.cpp:462
> -    WASM_VALIDATOR_FAIL_IF(argumentCount != args.size() - 1, "arity mismatch in call_indirect, got ", args.size() - 1, " arguments, expected ", argumentCount);
> +    RELEASE_ASSERT(argumentCount == args.size() - 1);

why?

> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:58
> +    // TODO: Handle allocation failure...

FIXME, not TODO. Was this meant to be done in this patch? If not, please create and link a bug

> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:64
> +    static_assert(sizeof(JSValue) == sizeof(CPURegister), "FIXME: the code below relies on this.");

FIXME?

> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:112
> -    unsigned totalFrameSize = registersToSpill.size() * sizeof(void*);
> -    totalFrameSize += WasmCallingConvention::headerSizeInBytes();
> -    totalFrameSize -= sizeof(CallerFrameAndPC);
> -    unsigned numGPRs = 0;
> -    unsigned numFPRs = 0;
> -    bool argumentsIncludeI64 = false;
> -    for (unsigned i = 0; i < signature.argumentCount(); i++) {
> -        switch (signature.argument(i)) {
> -        case Wasm::I64:
> -            argumentsIncludeI64 = true;
> -            FALLTHROUGH;
> -        case Wasm::I32:
> -        case Wasm::Anyref:
> -        case Wasm::Funcref:
> -            if (numGPRs >= wasmCallingConvention().m_gprArgs.size())
> -                totalFrameSize += sizeof(void*);
> -            ++numGPRs;
> -            break;
> -        case Wasm::F32:
> -        case Wasm::F64:
> -            if (numFPRs >= wasmCallingConvention().m_fprArgs.size())
> -                totalFrameSize += sizeof(void*);
> -            ++numFPRs;
> -            break;
> -        default:
> -            RELEASE_ASSERT_NOT_REACHED();
> -        }
> -    }
> +    size_t totalFrameSize = registersToSpill.size() * sizeof(CPURegister);
> +    CallInformation wasmFrameConvention = wasmCallingConvention().callLocationsFor(signature);
> +    RegisterAtOffsetList savedResultRegisters = wasmFrameConvention.computeResultsOffsetList();
> +    totalFrameSize += wasmFrameConvention.headerAndArgumentStackSizeInBytes;
> +    totalFrameSize += savedResultRegisters.size() * sizeof(CPURegister);

👌

> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:145
> +        CallInformation jsFrameConvention = jsCallingConvention().callLocationsFor(signature, JSCallingConvention::Callee);

looking at this used more and more, I think calling it "callingConventionFor" or "callInformationFor" is a much better name

> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:228
> +            jit.moveTrustedValue(jsNumber(pureNaN()), dst);
> +            auto isNaN = jit.branchIfNaN(src.fpr());
> +            jit.boxDouble(src.fpr(), dst, DoNotHaveTagRegisters);
> +            isNaN.link(&jit);

what if src == dst? Is that not possible? Can you assert?

... Actually, it looks wrong, when fpr argument is 0, they're the same register.

> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:240
> +    else if (signature.returnCount() < 2)
> +        boxWasmResult(signature.returnType(0), wasmFrameConvention.results[0].reg(), JSValueRegs { GPRInfo::returnValueGPR });

why not == 1? Seems like a nicer check. Since returnCount() == 0 implies void which you have above.

> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:242
> +

nit: remove line

> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:253
> +                jit.storeValue(scratch, CCallHelpers::Address(CCallHelpers::stackPointerRegister, savedResultRegisters.find(rep.reg())->offset() + wasmFrameConvention.headerAndArgumentStackSizeInBytes));

so we save space on the stack to do this which is contiguous with the region we have below?

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:420
> +            jit.moveTrustedValue(jsNumber(pureNaN()), dst);
> +            auto isNaN = jit.branchIfNaN(src.fpr());
> +            jit.boxDouble(src.fpr(), dst, DoNotHaveTagRegisters);
> +            isNaN.link(&jit);

same comment. Seems wrong when using the first fp argument register? it'll just overwrite dest with NaN? Is this not tested?

[Saam Barati]

Comment on attachment 379614 [details]
Just code changes

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

>> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:420
>> +            isNaN.link(&jit);
> 
> same comment. Seems wrong when using the first fp argument register? it'll just overwrite dest with NaN? Is this not tested?

ignore me. This is a gpr...

[Saam Barati]

Comment on attachment 379614 [details]
Just code changes

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

LGTM, I just have one question on how you're dealing with the multi return values which are in registers and how you pass in stack pointer and use the offset() to peer into their stack slots. Some of your math seems slightly inconsistent. It's likely I'm wrong, but just want to make sure we got it right

> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:48
> +JSArray* allocateResultsArray(ExecState* exec, Wasm::Instance* instance, const Signature* signature, IndexingType indexingType, JSValue* values)

it's not obvious from reading the code here that "values" is really stack pointer.

It's probably worth making it obvious, otherwise that loop below seems super crazy

>> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:240
>> +        boxWasmResult(signature.returnType(0), wasmFrameConvention.results[0].reg(), JSValueRegs { GPRInfo::returnValueGPR });
> 
> why not == 1? Seems like a nicer check. Since returnCount() == 0 implies void which you have above.

this comment was meant to be on the "else if" one line above

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:-149
> -    if (!Options::useCallICsForWebAssemblyToJSCalls()) {

let's delete this option from OptionsList

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:418
> +

remove newline

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:443
> +                        unboxedValue = value.toInt32(exec);
> +                        break;
> +                    case F32:
> +                        unboxedValue = bitwise_cast<uint32_t>(value.toFloat(exec));
> +                        break;
> +                    case F64:
> +                        unboxedValue = bitwise_cast<uint64_t>(value.toNumber(exec));
> +                        break;

don't you need to release the exception scope before these?

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:460
> +                        registerResults[registerResultOffsets.find(rep.reg())->offset() / sizeof(uint64_t)] = unboxedValue;

here you don't add frame size where in the other cases you do? I'm confused by some of your math which accesses this offset().

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:400
> +    auto boxWasmResult = [&] (Wasm::Type type, Reg src, JSValueRegs dst) {

can we share this code with the other boxWasmResult?

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:475
> +    if (signature.returnsVoid())
> +        jit.moveTrustedValue(jsUndefined(), JSValueRegs { GPRInfo::returnValueGPR });
> +    else if (signature.returnCount() == 1)
> +        boxWasmResult(signature.returnType(0), wasmCallInfo.results[0].reg(), JSValueRegs { GPRInfo::returnValueGPR });
> +    else {
> +        IndexingType indexingType = ArrayWithUndecided;
> +        JSValueRegs scratch = JSValueRegs { Wasm::wasmCallingConvention().prologueScratchGPRs[1] };
> +        // We can use the first floating point register as a scratch since it will always be moved onto the stack before other values.
> +        FPRReg fprScratch = Wasm::wasmCallingConvention().fprArgs[0].fpr();
> +        for (unsigned i = 0; i < signature.returnCount(); ++i) {
> +            B3::ValueRep rep = wasmCallInfo.results[i];
> +            Wasm::Type type = signature.returnType(i);
> +
> +            if (rep.isReg()) {
> +                boxWasmResult(signature.returnType(i), rep.reg(), scratch);
> +                jit.storeValue(scratch, CCallHelpers::Address(CCallHelpers::stackPointerRegister, savedResultRegisters.find(rep.reg())->offset() + wasmCallInfo.headerAndArgumentStackSizeInBytes));
> +            } else {
> +                auto location = CCallHelpers::Address(CCallHelpers::stackPointerRegister, rep.offsetFromSP());
> +                Reg tmp = type == Wasm::F32 || type == Wasm::F64 ? Reg(fprScratch) : Reg(scratch.gpr());
> +                jit.load64ToReg(location, tmp);
> +                boxWasmResult(signature.returnType(i), tmp, scratch);
> +                jit.storeValue(scratch, location);
> +            }
> +
> +            switch (type) {
> +            case Wasm::I32:
> +                indexingType = leastUpperBoundOfIndexingTypes(indexingType, ArrayWithInt32);
> +                break;
> +            case Wasm::F32:
> +            case Wasm::F64:
> +                indexingType = leastUpperBoundOfIndexingTypes(indexingType, ArrayWithDouble);
> +                break;
> +            default:
> +                indexingType = leastUpperBoundOfIndexingTypes(indexingType, ArrayWithContiguous);
> +                break;
> +            }
> +        }
> +
> +        GPRReg wasmContextInstanceGPR = pinnedRegs.wasmContextInstancePointer;
> +        if (Wasm::Context::useFastTLS()) {
> +            wasmContextInstanceGPR = GPRInfo::argumentGPR1;
> +            static_assert(std::is_same_v<Wasm::Instance*, typename FunctionTraits<decltype(Wasm::allocateResultsArray)>::ArgumentType<1>>, "Instance should be the second parameter.");
> +            jit.loadWasmContextInstance(wasmContextInstanceGPR);
> +        }
> +
> +        jit.setupArguments<decltype(Wasm::allocateResultsArray)>(wasmContextInstanceGPR, CCallHelpers::TrustedImmPtr(&signature), indexingType, CCallHelpers::stackPointerRegister);
> +        jit.callOperation(FunctionPtr<OperationPtrTag>(Wasm::allocateResultsArray));

all the code here looks identical to the wasm entrypoint wrapper's code. Can we share it?

[Keith Miller]

Comment on attachment 379614 [details]
Just code changes

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

>> Source/JavaScriptCore/ChangeLog:18
>> +        way by using the fact that the old value type trivial signature
> 
> this sentence needs some grammar editing

Fixed.

>> Source/JavaScriptCore/ChangeLog:35
>> +        calling conventions code and moves that logic into the Air IR
> 
> conventions => convention

Fixed.

>> Source/JavaScriptCore/b3/B3StackmapGenerationParams.h:99
>> +    // These are provided for convenience; they means that you don't have to capture them if you don't want to.
> 
> "they means" => "they mean"

Fixed.

>> Source/JavaScriptCore/jit/AssemblyHelpers.h:92
>> +    }
> 
> do we really need to names these "fromReg"?

I went with fromReg because I didn't want to end up having someone pass a fpr to the store32 function and not have it be typed checked.

>> Source/JavaScriptCore/runtime/JSCConfig.h:43
>> +constexpr size_t ConfigSizeToProtect = 32 * KB;
> 
> :(
> 
> Do we really have this many options?

Ugh, yeah... We need to make it much smaller. I'll work on that after this is done.

>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:424
>> +        emitPatchpoint(basicBlock, patch, Vector<Tmp, 1> { result }, Vector<ConstrainedTmp, sizeof...(Args)>::from(theArgs...));
> 
> nit: I wonder if just "{ result }" would work

Nope, Sadly. I tried...

>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:432
>> +    template <typename ResultTmpType, size_t inlineSize>
> 
> why is ResultTmpType a template type? It's not static?

Sometimes we pass a Tmp sometimes we pass a TypedTmp.

>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:496
>> +                // If the patch is a terminal this is a return.
> 
> to be pedantic: "a return" => "a multivalve return"

Actually this code doesn't need to exist because return patchpoints don't pass StackArguments anymore... Will delete.

>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2033
>> +    CallInformation locations = wasmCallingConvention().callLocationsFor(signature);
> 
> nit: name of this function and variable seems weird. Why not call the API "callInformationFor" instead of "callLocationsFor"?

Yeah, a can change that. I think I changed it several times when building this patch, which is why it's kinda inconsistent across files.

>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2040
>> +    patchArgs.grow(newSize.unsafeGet());
> 
> nit: can't you just call the "get" that crashes if overflow instead of the assert?

I don't think there is a get?

>> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2953
>> +    emitPatchpoint(m_currentBlock, patchpoint, Vector<Tmp, 1> { result }, WTFMove(args));
> 
> same comment as earlier. I wonder if "{ result }" will Just Work here

Ditto.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:238
>> +//    PartialResult WARN_UNUSED_RETURN splitStack(BlockSignature, Stack& currentStack, Stack& newStack);
> 
> please remove

Whoops removed.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1172
>> +static B3IRGenerator::Stack splitStack(BlockSignature signature, B3IRGenerator::Stack& stack)
> 
> can this be written in such a way that it's agnostic of Air vs B3. Essentially identical code exists in the Air IR generator too

Done, I put it in WasmFunctionParser.h

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1443
>> +    ASSERT(returnValues.size() >= wasmCC.results.size());
> 
> nit: release assert

Done.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1446
>> +        auto rep = wasmCC.results[i];
> 
> nit: type name or a longer variable name here is nicer IMO

done.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1546
>> +    constrainedArguments.append(B3::ConstrainedValue(args[i], wasmCC.params[i]));
> 
> indent is wrong

Fixed

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1578
>> +            for (unsigned i = 0; i < signature.returnCount(); ++i)
> 
> can we assert that these numbers are the same size? Should they be?

Sure, that's easy to do.

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1845
>> +    }
> 
> why not abstract "fillResults" lambda above?

Can you clarify what you mean here? I don't follow.

>> Source/JavaScriptCore/wasm/WasmCallingConvention.cpp:78
>> +        WTF::storeStoreFence();
> 
> why? Seems unneeded. Seems like std::once would be wrong if this was needed

I guess that's a good point... I think I was thinking that std::once would work by testing if the value was non-null then just using it if not. In that case you'd need the fence here to ensure consistency (I think).

>> Source/JavaScriptCore/wasm/WasmCallingConvention.h:91
>> +    enum CallMode : bool {
> 
> enum class?

I followed the comment below and moved it out of the two classes. 

FYI, It was originally a bool just so that it had a name if you wanted to override the default parameter.

>> Source/JavaScriptCore/wasm/WasmCallingConvention.h:93
>> +        Callee = false,
> 
> do we actually rely on the values here?

Yeah, I was using it below. I don't rely on it anymore.

>> Source/JavaScriptCore/wasm/WasmParser.h:284
>> +    WASM_PARSER_FAIL_IF(!Options::useWebAssemblyMultiValues(), "Type table indicies for block signatures are not supported yet");
> 
> "indicies" => "indices"

Fixed.

>> Source/JavaScriptCore/wasm/WasmParser.h:287
>> +    WASM_PARSER_FAIL_IF(!parseVarInt64(index), "Block-like instruction doesn't return value type but can't decode length");
> 
> nit: this error message could be made slightly more clear IMO

Fixed.

>> Source/JavaScriptCore/wasm/WasmSignature.cpp:164
>> +    }
> 
> what's this used for?

This is so that we can find where a signature would be located in the table without constructing one. Basically, it's just a duplicate of the hash function that Signatures internally implement.

>> Source/JavaScriptCore/wasm/WasmValidate.cpp:462
>> +    RELEASE_ASSERT(argumentCount == args.size() - 1);
> 
> why?

Because we remove these items in the function parser. So it should be the right size already.

>> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:48
>> +JSArray* allocateResultsArray(ExecState* exec, Wasm::Instance* instance, const Signature* signature, IndexingType indexingType, JSValue* values)
> 
> it's not obvious from reading the code here that "values" is really stack pointer.
> 
> It's probably worth making it obvious, otherwise that loop below seems super crazy

Sure.

>> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:58
>> +    // TODO: Handle allocation failure...
> 
> FIXME, not TODO. Was this meant to be done in this patch? If not, please create and link a bug

Yeah, sorry. Will do a FIXME.

>> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:64
>> +    static_assert(sizeof(JSValue) == sizeof(CPURegister), "FIXME: the code below relies on this.");
> 
> FIXME?

I guess we may not

>> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:228
>> +            isNaN.link(&jit);
> 
> what if src == dst? Is that not possible? Can you assert?
> 
> ... Actually, it looks wrong, when fpr argument is 0, they're the same register.

dst is a GPR so they can't be the same for the floating point cases.

>>> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:240
>>> +        boxWasmResult(signature.returnType(0), wasmFrameConvention.results[0].reg(), JSValueRegs { GPRInfo::returnValueGPR });
>> 
>> why not == 1? Seems like a nicer check. Since returnCount() == 0 implies void which you have above.
> 
> this comment was meant to be on the "else if" one line above

Done.

>> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:253
>> +                jit.storeValue(scratch, CCallHelpers::Address(CCallHelpers::stackPointerRegister, savedResultRegisters.find(rep.reg())->offset() + wasmFrameConvention.headerAndArgumentStackSizeInBytes));
> 
> so we save space on the stack to do this which is contiguous with the region we have below?

Yeah, we save space just above the last argument/result, whichever is highest on the stack.

>> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:-149
>> -    if (!Options::useCallICsForWebAssemblyToJSCalls()) {
> 
> let's delete this option from OptionsList

Done.

>> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:460
>> +                        registerResults[registerResultOffsets.find(rep.reg())->offset() / sizeof(uint64_t)] = unboxedValue;
> 
> here you don't add frame size where in the other cases you do? I'm confused by some of your math which accesses this offset().

That's because the wasm->js variant writes stack values directly to the caller's stack and register parameters to space reserved just above SP.

>> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:400
>> +    auto boxWasmResult = [&] (Wasm::Type type, Reg src, JSValueRegs dst) {
> 
> can we share this code with the other boxWasmResult?

Sure.

>> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:475
>> +        jit.callOperation(FunctionPtr<OperationPtrTag>(Wasm::allocateResultsArray));
> 
> all the code here looks identical to the wasm entrypoint wrapper's code. Can we share it?

done

[Keith Miller]

Created attachment 379842 [details]
Patch

[Saam Barati]

Comment on attachment 379842 [details]
Patch

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

r=me

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:864
> +    CallInformation wasmCC = wasmCallingConvention().callLocationsFor(signature, CallRole::Callee);

Still not a fan of the name “callLocatjonsFor”

[Keith Miller]

Created attachment 379906 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 379906 [details]
Patch for landing

Clearing flags on attachment: 379906

Committed r250559: <https://trac.webkit.org/changeset/250559>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55879128>