[webkit-reviews] review granted: [Bug 194257] [WebAssembly] Create a Wasm interpreter : [Attachment 382432] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Tadeu Zagallo
<tzagallo at apple.com>'s request for review:
Bug 194257: [WebAssembly] Create a Wasm interpreter
https://bugs.webkit.org/show_bug.cgi?id=194257

Attachment 382432: Patch

https://bugs.webkit.org/attachment.cgi?id=382432&action=review




--- Comment #40 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 382432
  --> https://bugs.webkit.org/attachment.cgi?id=382432
Patch

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

r=me

> Source/JavaScriptCore/llint/WebAssembly.asm:701
> +	       # store all argument registers to the stack, they might have
return values

the part that's unclear is why argument registers might have return values.

> Source/JavaScriptCore/llint/WebAssembly.asm:821
> +	   cdqi

can we static assert t0 is rax?

> Source/JavaScriptCore/llint/WebAssembly.asm:823
> +	   returni(ctx, t0)

this can go. outside the "if"

> Source/JavaScriptCore/llint/WebAssembly.asm:826
> +	   returni(ctx, t0)

ditto

> Source/JavaScriptCore/llint/WebAssembly.asm:878
> +wasmOp(i32_rem_s, WasmI32RemS, macro (ctx)
> +    mloadi(ctx, m_lhs, t0)
> +    mloadi(ctx, m_rhs, t1)
> +
> +    btiz t1, .throwDivisionByZero
> +
> +    bineq t1, -1, .safe
> +    bineq t0, constexpr INT32_MIN, .safe
> +
> +    move 0, t2
> +    jmp .return
> +
> +.safe:
> +    if X86_64
> +	   cdqi
> +	   idivi t1
> +    else
> +	   divis t1, t0, t2
> +	   muli t1, t2
> +	   subi t0, t2, t2
> +    end
> +
> +.return:
> +    returni(ctx, t2)

can we make a macro and share code here with `i32_div_s`? I think the safety
checks are identical

> Source/JavaScriptCore/llint/WebAssembly.asm:965
> +wasmOp(i64_rem_s, WasmI64RemS, macro (ctx)

ditto about sharing code with i64 div_s

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:-1718
> -	   const auto& results =
m_parser->controlStack()[controlIndex].controlData.results;
> -	   for (auto& value : results)
> -	       patchArgs.append(ConstrainedTmp(value, B3::ValueRep::ColdAny));

why?

> Source/JavaScriptCore/wasm/WasmCallee.cpp:95
> +	   registers.set(GPRInfo::regCS0); // Wasm::Instance
> +#if CPU(X86_64)
> +	   registers.set(GPRInfo::regCS2); // PB
> +#else
> +	   registers.set(GPRInfo::regCS7); // PB
> +#endif
> +	   registers.set(GPRInfo::regCS3); // Memory base
> +	   registers.set(GPRInfo::regCS4); // Memory size

can we unify this with the calling convention struct? It seems weird to not
just specify a constant in both cases.

> Source/JavaScriptCore/wasm/WasmCodeBlock.cpp:75
> +		   m_llintCallees[calleeIndex] =
adoptRef(static_cast<LLIntCallee*>(wasmEntrypoint.leakRef()));
> +	       else
> +		   m_bbqCallees[calleeIndex] =
adoptRef(static_cast<BBQCallee*>(wasmEntrypoint.leakRef()));

why not just WTFMove()?

> Source/JavaScriptCore/wasm/WasmFunctionCodeBlock.cpp:43
> +void FunctionCodeBlock::setInstructions(std::unique_ptr<InstructionStream>
instructions)
> +{
> +    m_instructions = WTFMove(instructions);
> +    m_instructionsRawPointer = m_instructions->rawPointer();
> +}

This class seems like it should just be merged with WasmCallee

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:523
> +// Globals

nit: comment not needed

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:541
> +
> +

nit: extra newline

> Source/JavaScriptCore/wasm/WasmLLIntPlan.cpp:130
> +#if CPU(X86_64)
> +	       CCallHelpers::Address
calleeSlot(CCallHelpers::stackPointerRegister, CallFrameSlot::callee *
static_cast<int>(sizeof(Register)) - sizeof(CPURegister));
> +#else
> +	       CCallHelpers::Address
calleeSlot(CCallHelpers::stackPointerRegister, CallFrameSlot::callee *
static_cast<int>(sizeof(Register)) - sizeof(CallerFrameAndPC));
> +#endif

you have code like this in a few places, but I'd make it something like:

#elif CPU(ARM64)
...
#else
#error "bad architecture"
#endif