Bug 161569

Created attachment 287866 [details] WIP

Created attachment 288032 [details] Patch

Comment on attachment 288032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288032&action=review > Source/JavaScriptCore/wasm/WASMFunctionParser.h:173 > + case OpType::Loop: { > + if (!m_context.addLoop()) > + return false; > + return parseBlock(); > + } I'll fix this indentation before landing.

Comment on attachment 288032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288032&action=review > Source/JavaScriptCore/ChangeLog:19 > + Another interesting thing of note is that we now only allocate the > + continuation basic block. This is beneficial when the continuation we now only allocate the continuation basic block *lazily*? > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:48 > +static const bool verbose = false; In B3, we started using anonymous namespaces like LLVM. This would be in the anonymous namespace instead of being a static variable. > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:74 > + LazyBlock(BasicBlock* block) You could have LazyBlock() = default; to simplify ControlData. > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:79 > + explicit operator bool() { return m_block; } !!m_block > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:97 > + BasicBlock* m_block; Can be BasicBlock* m_block { nullptr; } > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:102 > + ControlData(Optional<Vector<Variable*>>&& _stack, BasicBlock* _loopTarget = nullptr) What's with the "_" prefix of the variables? Shouldn't there be a ControlData() = default; too for when you start one of those? > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:103 > + : continuation(nullptr) This could go away if you do the changes above. > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:125 > + bool isLoop() { return loopTarget; } const + !!loopTarget > Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:289 > + return true; Since you are not changing m_currentBlock, wouldn't you add instruction past the terminal? Say: (block (block (foo) (return) ) (bar) )

Comment on attachment 288032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288032&action=review > Source/JavaScriptCore/wasm/WASMCallingConvention.cpp:1 > +/* Please add this new file to the cmake buildsystem too - Source/JavaScriptCore/CMakeLists.txt (Don't need to remove B3CallingConventions.cpp, because it wasn't added ever.) > Source/JavaScriptCore/wasm/WASMCallingConvention.cpp:31 > +#include <wtf/NeverDestroyed.h> > + > +#if ENABLE(WEBASSEMBLY) Please move this include after the #if guard.

Comment on attachment 288032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288032&action=review > Source/JavaScriptCore/wasm/WASMCallingConvention.cpp:35 > +namespace JSC { > + > +namespace WASM { Use namespace JSC { namespace WASM { style similar to all JSC sources. It would be great if you could fix this style in other WASM sources too. > Source/JavaScriptCore/wasm/WASMCallingConvention.cpp:50 > +} // namespace B3 > + > +} // namespace JSC } } // namespace JSC::WASM > Source/JavaScriptCore/wasm/WASMCallingConvention.h:40 > +namespace JSC { > + > +namespace WASM { ditto > Source/JavaScriptCore/wasm/WASMCallingConvention.h:92 > +} // namespace WASM > + > +} // namespace JSC ditto

Comment on attachment 288032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288032&action=review >> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:48 >> +static const bool verbose = false; > > In B3, we started using anonymous namespaces like LLVM. This would be in the anonymous namespace instead of being a static variable. Changed. >> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:102 >> + ControlData(Optional<Vector<Variable*>>&& _stack, BasicBlock* _loopTarget = nullptr) > > What's with the "_" prefix of the variables? > > Shouldn't there be a ControlData() = default; too for when you start one of those? Whoops deleted. Right now we don't need one. Although when I add if I'll need to add one then. >> Source/JavaScriptCore/wasm/WASMB3IRGenerator.cpp:289 >> + return true; > > Since you are not changing m_currentBlock, wouldn't you add instruction past the terminal? > > Say: > (block > (block > (foo) > (return) > ) > (bar) > ) Whoops I forgot to add the code that prevents adding instructions when we are in unreachable. The change is to add a check at the top of parseExpression that just returns if we are in unreachable code. Unless the value is a control value.

Comment on attachment 288032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288032&action=review >> Source/JavaScriptCore/wasm/WASMCallingConvention.cpp:31 >> +#if ENABLE(WEBASSEMBLY) > > Please move this include after the #if guard. Fixed. >> Source/JavaScriptCore/wasm/WASMCallingConvention.cpp:35 >> +namespace WASM { > > Use namespace JSC { namespace WASM { style similar to all JSC sources. > It would be great if you could fix this style in other WASM sources too. done.

Committed r205552: <http://trac.webkit.org/changeset/205552>