[wasm2c] Add support for i32 arithmetic and basic variables - #8957
[wasm2c] Add support for i32 arithmetic and basic variables#8957lexi-nadia wants to merge 1 commit into
Conversation
This change adds the bare minimum support needed to make the i32.wast spec test suite pass.
| isFirst = false; | ||
| } else if (c == '_') { | ||
| if (isFirst) { | ||
| result += "0x5F"; |
There was a problem hiding this comment.
Wouldn't this create an identifier starting with a number? That's not valid, right? I see that this is only used to mangle some suffix of the final name. It would be good to add a comment about that.
| std::vector<std::string> modulePrefixes; | ||
| std::unordered_map<Name, std::string> moduleNameToPrefix; | ||
| std::unordered_map<std::string, std::unordered_set<std::string>> | ||
| moduleExports; | ||
| std::string lastModulePrefix; |
There was a problem hiding this comment.
It would be good to have some comments on what these hold. In particular it would be good to point out that we may be translating multiple modules at once for spec tests that contain multiple modules.
| // Verify export exists | ||
| auto expIt = moduleExports.find(activePrefix); | ||
| if (expIt == moduleExports.end() || | ||
| !expIt->second.count(invoke->name.toString())) { |
There was a problem hiding this comment.
| !expIt->second.count(invoke->name.toString())) { | |
| !expIt->second.contains(invoke->name.toString())) { |
| exports.insert(exp->name.toString()); | ||
| } | ||
| } | ||
| moduleExports[prefix] = exports; |
There was a problem hiding this comment.
| moduleExports[prefix] = exports; | |
| moduleExports[prefix] = std::move(exports); |
|
|
||
| // Verify export exists | ||
| auto expIt = moduleExports.find(activePrefix); | ||
| if (expIt == moduleExports.end() || |
There was a problem hiding this comment.
It looks like we could do something like const auto& exports = moduleExports.at(activePrefix); because we already verified that the module name is good and therefore activePrefix must exist and have an entry in moduleExports.
| : c(c), func(func), moduleName(moduleName) {} | ||
|
|
||
| void push() { | ||
| stackDepth++; |
There was a problem hiding this comment.
We generally prefer pre-increment operators (although this was not historically the case).
| stackDepth++; | |
| ++stackDepth; |
| if (func->isParam(curr->index)) { | ||
| name = "var_p" + std::to_string(curr->index); | ||
| } else { | ||
| name = "var_l" + std::to_string(curr->index - func->getNumParams()); | ||
| } |
There was a problem hiding this comment.
Any particular reason not to unify the names and just use var_<i> for both?
| if (exportedFunctions.count(func->name)) { | ||
| std::string exportedName = mangleName(exportedFunctions[func->name]); |
There was a problem hiding this comment.
Can avoid the double lookup here and similar places.
| std::string exportedName = mangleName(exportedFunctions[func->name]); | ||
| c << resType << " w2c_" << moduleName << "_" << exportedName << "(" | ||
| << paramsDecl << ") {" << endl; |
There was a problem hiding this comment.
It would be nice to pull out helper functions for all the different kinds of names that need to be assembled.
| StackIR* stackIR = moduleStackIR.getStackIROrNull(func.get()); | ||
| if (!stackIR) { | ||
| Fatal() << "Failed to generate Stack IR for function " << func->name; | ||
| } |
There was a problem hiding this comment.
Since we just need to visit each instruction in order, it would be simpler to use a PostWalker than StackIR. Unless there's another reason to use StackIR that I'm forgetting?
2 participants
This change adds the bare minimum support needed to make the i32.wast spec test suite pass.