MCContext does not destroy MCSymbols on shutdown. So, rather than putting SmallVectors (which may heap-allocate) inside MCSymbolWasm, use unowned pointer to a WasmSignature instead. The signatures are now owned by the AsmPrinter.
Also uses WasmSignature instead of param and result vectors in TargetStreamer, and leaves some TODOs for further simplification.
Details
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 23264 Build 23263: arc lint + arc unit
Event Timeline
Yeah sorry this still needs some cleanup before it's ready for review (also I need to post the lld side) . arc's --plan-changes flag claims to upload without requesting review (and it does) but unfortunately it doesn't suppress the herald rules :/
Ah, yeah I saw "dschuff planned changes to this revision." in phabricator and wasn't entirely sure what that meant, but didn't let that slow me down
lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp | ||
---|---|---|
157 | With this patch as it is now, the WasmStreamer's emitParam no longer does anything, but the AsmStreamer does. This weakens the mental model of the AsmPrinter somewhat, because code like EmitFunctionBodyStart here is conceptually supposed to be "emitting assembly code" through directives, with those directives mapping to either emitting the actual text for the directives, or implementing the directives directly in binary. This mental model is useful for keeping the codegen to object file path in sync with the AsmParser to object file path. Since I don't see a call to setSignature in the AsmParser to object file path, is it not currently supporting signatures with this patch? |
Generally looks good. Yay for unblocking us from leaving experimental
include/llvm/Object/WasmTraits.h | ||
---|---|---|
27 | So we were just using 1 and 2 here as integers that happen to not be wasm value types? | |
lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp | ||
238–240 | Maybe makes sense to move this to lib/BinaryFormat/Wasm.cpp now? Since the other ToString methods for the bin format live there. Although perhaps its good to have all the assembly string constants here? Seems a little odd that the exact save strings are returned by the above function, maybe they can be unified somehow? | |
lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp | ||
150–152 | Why not set the type of FUNCTION in setSignature()? And assert here maybe for sanity check? | |
lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp | ||
90–93 | I suppose this rename could be a separate NFC? Although maybe not worth it. | |
tools/obj2yaml/wasm2yaml.cpp | ||
160 | Should we assert there that size() <= 1? |
include/llvm/Object/WasmTraits.h | ||
---|---|---|
27 | Yeah, it looks that way. | |
lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp | ||
238–240 | Yeah probably ExprType and ValType could be unified. There is maybe a little bit better organization or separation we could have between the raw enums in BinaryFormat/Wasm.h and ValType (and MVT), but I tried to keep that to a minimum for this CL. Also I don't know why the BinaryFormat functions are in namespace wasm and the other stuff is in namespace WebAssembly. Maybe for now we can keep them here (so at least the strings in the data section can stay unified) and then fix the above in a follow-up. | |
lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp | ||
150–152 | For every other case it's set in EmitSymbolAttribute. Indirect function type directives are just weird because the symbol we are emitting corresponds to an undefined function. This is another thing we might rethink now that s2wasm is gone and we just have our own asm parser and printer to coordinate. | |
lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp | ||
157 | Yeah I agree this is weird and should be fixed, although I decided not to take that any further for this CL. We can probably replace both of them with something else (i.e. different scheme of assembler directives) if that makes more sense. I had actually forgotten that there was even AsmParser support for .param directives, since the assembly language is still in flux and we aren't actually using it anywhere right now. And come to think of it we'll have to handle the ownership of signatures similarly (i.e. outside of the MCContext, perhaps on the AsmParser). I think what we'll want to do is get rid of the separate .param and .result directives in favor of a full signature specifier; something more like what we have already for .functype (but make it able to handle multiple returns). I don't know of anywhere we'd need to use just params or just results. Not sure if it's better to do that in a followup CL (better for review, but would undo a bit of this CL) or pack it into this one. | |
lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp | ||
90–93 | done in rL343275 |
- Merge branch 'master' into signatures
- add assert
- Don't compute signature in EmitEndOfAsmFile if one is already there
- Handle SRet in ComputeSignatureVTs
- Fix -Wcovered-switch-default
- Use ComputeSignatureVTs in more places
PTAL
lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp | ||
---|---|---|
157 | OK, I went further on this, and it's better. I think we should go all the way and remove the separate .param/.result but that requires changing all the tests, so it should be a separate CL. |
LGTM. In the future it's possible we could need to hold the signatures somewhere other than the AsmPrinter, however we can figure that out later, when it's more clear what the MC AsmParser layer will need.
I tried to added .functype parsing support to WebAssemblyAsmParser and it involves calling MCSymbolWasm::setSignature from WebAssemblyAsmParser. I guess we should move the ownership to somewhere else. Here is the bug report for .functype handling.
So we were just using 1 and 2 here as integers that happen to not be wasm value types?