We do this wrong at the moment.
Created attachment 293693 [details] WIP
Created attachment 293753 [details] Patch
Comment on attachment 293753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293753&action=review > Source/JavaScriptCore/ChangeLog:10 > + return value. Most of the control flow operators needed to be rewritten in able to "in able to" > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:84 > + ControlData(Procedure& proc, Type signature, BlockType type, BasicBlock* continuation, BasicBlock* special = nullptr) What's special? > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:571 > + entry.expressionStack.append(m_currentBlock->appendNew<VariableValue>(m_proc, B3::Get, Origin(), result)); This can fail, right? I think we should use all the `try` variants everywhere so we soft-fail and bail out. > Source/JavaScriptCore/wasm/WasmFunctionParser.h:54 > + ExpressionList expressionStack; Rename this to avoid the comment above. > Source/JavaScriptCore/wasm/WasmFunctionParser.h:158 > + returnValues.append(returnValue); Same on allocation. > Source/JavaScriptCore/wasm/WasmFunctionParser.h:314 > + m_controlStack.append({ WTFMove(m_expressionStack), m_context.addBlock(inlineSignature) }); ditto (and then the other ones below, I'm lazy) > Source/JavaScriptCore/wasm/WasmValidate.cpp:319 > + // Think of this as penance for the sin of bad error messages. ?
Comment on attachment 293753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293753&action=review >> Source/JavaScriptCore/ChangeLog:10 >> + return value. Most of the control flow operators needed to be rewritten in able to > > "in able to" /rewritten in able to/rewritten to/. > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:132 > + void makeBlock() > + { > + ASSERT(type() == BlockType::If); > + blockType = BlockType::Block; > + special = nullptr; Why is this assert true? Seems pretty random. If the sole purpose of this function is to convert an If ControlData to a Block ControlData, I suggest renaming the function to "convertIfToBlock" or "makeBlockFromIf". That would make the assertion meaningful.
Comment on attachment 293753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293753&action=review >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:132 >> + special = nullptr; > > Why is this assert true? Seems pretty random. If the sole purpose of this function is to convert an If ControlData to a Block ControlData, I suggest renaming the function to "convertIfToBlock" or "makeBlockFromIf". That would make the assertion meaningful. Sure, I'll change it to convertIfToBlock. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:571 >> + entry.expressionStack.append(m_currentBlock->appendNew<VariableValue>(m_proc, B3::Get, Origin(), result)); > > This can fail, right? I think we should use all the `try` variants everywhere so we soft-fail and bail out. Yeah, that's a fair point. I think we should do that in a follow up patch though. >> Source/JavaScriptCore/wasm/WasmFunctionParser.h:54 >> + ExpressionList expressionStack; > > Rename this to avoid the comment above. Fair enough. I changed it to enclosedExpressionStack.
Created attachment 293771 [details] Patch
Comment on attachment 293771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293771&action=review r=me with a couple comments > Source/JavaScriptCore/testWasm.cpp:310 > + Vector<uint8_t> vector = { It'll be nice once we can make tests not using this API. > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:611 > + for (size_t i = 0; i < result.size(); ++i) This loop doesn't look right to me. I think you want to be going from top of stack downwards on both, for result.size() iterations. Maybe something like: (unsigned i = 0; i < result.size(); i++) unify(result[result.size() - 1 - i], resultStack[resultStack.size() - 1 - i) Can you add a test for this? > Source/JavaScriptCore/wasm/WasmFunctionParser.h:146 > + RELEASE_ASSERT_NOT_REACHED(); Why is this RELEASE_ASSERT_NOT_REACHED, if it is, maybe the above if (!parseUnreachableExpression(static_cast<OpType>(op))) should be an assertion. > Source/JavaScriptCore/wasm/WasmFunctionParser.h:476 > + if (m_expressionStack.size()) { that makes sense. > Source/JavaScriptCore/wasm/WasmValidate.cpp:319 > + // Think of this as penance for the sin of bad error messages. lol
Comment on attachment 293771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293771&action=review >> Source/JavaScriptCore/testWasm.cpp:310 >> + Vector<uint8_t> vector = { > > It'll be nice once we can make tests not using this API. +1 >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:611 >> + for (size_t i = 0; i < result.size(); ++i) > > This loop doesn't look right to me. I think you want to be going from top of stack downwards on both, for result.size() iterations. > Maybe something like: > (unsigned i = 0; i < result.size(); i++) unify(result[result.size() - 1 - i], resultStack[resultStack.size() - 1 - i) > > Can you add a test for this? Yeah, that's right. It's a little tricky to add a test for this since the only time this is wrong is when the sizes are not equal. The current version of the ML-proto I'm using to generate tests does not support extra stack values. I'll file a bug for it though in the future. >> Source/JavaScriptCore/wasm/WasmFunctionParser.h:146 >> + RELEASE_ASSERT_NOT_REACHED(); > > Why is this RELEASE_ASSERT_NOT_REACHED, if it is, maybe the above if (!parseUnreachableExpression(static_cast<OpType>(op))) should be an assertion. We don't want the if to be an assert since it might fail in the validator. Also, every exit from the above loop is a return so unless there is a compiler bug this assert is trivially dead.
(In reply to comment #8) > Comment on attachment 293771 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293771&action=review > > >> Source/JavaScriptCore/testWasm.cpp:310 > >> + Vector<uint8_t> vector = { > > > > It'll be nice once we can make tests not using this API. > > +1 > > >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:611 > >> + for (size_t i = 0; i < result.size(); ++i) > > > > This loop doesn't look right to me. I think you want to be going from top of stack downwards on both, for result.size() iterations. > > Maybe something like: > > (unsigned i = 0; i < result.size(); i++) unify(result[result.size() - 1 - i], resultStack[resultStack.size() - 1 - i) > > > > Can you add a test for this? > > Yeah, that's right. It's a little tricky to add a test for this since the > only time this is wrong is when the sizes are not equal. The current version > of the ML-proto I'm using to generate tests does not support extra stack > values. I'll file a bug for it though in the future. I should note that updating the ml-proto would require changing all the opcode numbers in the existing tests. That would be an unfortunately large amount of work. > > >> Source/JavaScriptCore/wasm/WasmFunctionParser.h:146 > >> + RELEASE_ASSERT_NOT_REACHED(); > > > > Why is this RELEASE_ASSERT_NOT_REACHED, if it is, maybe the above if (!parseUnreachableExpression(static_cast<OpType>(op))) should be an assertion. > > We don't want the if to be an assert since it might fail in the validator. > Also, every exit from the above loop is a return so unless there is a > compiler bug this assert is trivially dead.
Created attachment 293786 [details] Patch for landing
Created attachment 293796 [details] Patch for landing
Initially committed in r208341: <http://trac.webkit.org/changeset/208341> Changelog was fixed in r208343: <http://trac.webkit.org/changeset/208343>