[webkit-reviews] review granted: [Bug 204474] [WebAssembly] Validate and generate bytecode in one pass : [Attachment 384236] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Dec 3 16:15:47 PST 2019
Saam Barati <sbarati at apple.com> has granted Tadeu Zagallo
<tzagallo at apple.com>'s request for review:
Bug 204474: [WebAssembly] Validate and generate bytecode in one pass
https://bugs.webkit.org/show_bug.cgi?id=204474
Attachment 384236: Patch
https://bugs.webkit.org/attachment.cgi?id=384236&action=review
--- Comment #4 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 384236
--> https://bugs.webkit.org/attachment.cgi?id=384236
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=384236&action=review
r=me
> Source/JavaScriptCore/ChangeLog:13
> + This is a 1.5x speedup when compiling the ZenGarden demo.
what do we do for the .validate API? (You should say here.)
> Source/JavaScriptCore/ChangeLog:14
> +
you should talk about the new policy that WebAssembly.module compiles bytecode
> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:237
> + static_assert(std::is_same_v<ResultList,
FunctionParser<AirIRGenerator>::ResultList>);
why not just define one to be the other?
> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:89
> + }
maybe ASSERT here that m_state == Validated
> Source/JavaScriptCore/wasm/WasmFunctionParser.h:146
> +#define WASM_VALIDATOR_FAIL_IF(condition, ...) do { \
> + if (UNLIKELY(condition)) \
> + return validationFail(__VA_ARGS__); \
> + } while (0)
style nit: line up the "\" or have them be one space after
> Source/JavaScriptCore/wasm/WasmFunctionParser.h:288
> + WASM_VALIDATOR_FAIL_IF(pointer.type() != I32, m_currentOpcode, " pointer
type mismatch");
maybe instead of "type mismatch" say " pointer type must be an i32"?
(This is probably not new to your patch though, so feel free to ignore)
> Source/JavaScriptCore/wasm/WasmFunctionParser.h:311
> + WASM_VALIDATOR_FAIL_IF(pointer.type() != I32, m_currentOpcode, " pointer
type mismatch");
ditto
> Source/JavaScriptCore/wasm/WasmLLIntPlan.cpp:58
> +{
ASSERT m_callees?
More information about the webkit-reviews
mailing list