[webkit-reviews] review granted: [Bug 204092] [WebAssembly] Improve Wasm::LLIntGenerator : [Attachment 384127] 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 204092: [WebAssembly] Improve Wasm::LLIntGenerator
https://bugs.webkit.org/show_bug.cgi?id=204092

Attachment 384127: Patch

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




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

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

r=me

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:360
> +	   });

nit: maybe checkConsistency after too?

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:363
> +    void materializeLocals(Stack& expressionStack, size_t newStackSize)

put inside splitStack? Or maybe call this materializeLocalsAfterSplit and name
"newStackSize" => "splitStackSize"?

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:403
> +    unsigned m_topLevelStackSize;

this seems completely unused (except setting it somewhere)

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:783
> +    materializeLocals(enclosingStack, newStack.size());

why not put materializeLocals into splitStack?

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:959
> +#if !ASSERT_DISABLED
> +	   ASSERT(unreachable || tmp == expressionStack[i]);
> +#endif

no need for !ASSERT_DISABLED here. Also, you should just ASSERT_UNUSED and
remove the UNUSED_PARAM above