[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.
| 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?
There was a problem hiding this comment.
As with many strange decisions, the answer is "because WABT's wasm2c does it that way".
There was a problem hiding this comment.
Are we still trying to match WABT's output as far as is reasonable?
If so, it would be good to at least pull this logic out into a helper.
| 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?
There was a problem hiding this comment.
The most fundamental issue is blocks with values (or other types of statement expressions, if they exist).
wasm2js has the same issue. It deals with it by using the Flatten pass, but Flatten doesn't support try_table or br_on_*, whereas StackIR does (AFAIK).
There was a problem hiding this comment.
Oh, StackIR won't fix that, though. If a block returns a value, it will still return a value when converted to StackIR.
2 participants
This change adds the bare minimum support needed to make the i32.wast spec test suite pass.