Also, some cleanup:
Use ManagedStatics instead of const tables to avoid memory/binary bloat.
Use a StringMap instead of a linear search for name lookup.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/Target/WebAssembly/WebAssemblyRuntimeLibcallSignatures.cpp | ||
---|---|---|
883 ↗ | (On Diff #107208) | Can't construct a StringRef will nullptr, so we have this bit of ugliness. |
lib/Target/WebAssembly/WebAssemblyRuntimeLibcallSignatures.cpp | ||
---|---|---|
883 ↗ | (On Diff #107208) | I don't mind much, but if you care, can we do something like this? enum Libcall { #define HANDLE_LIBCALL(code, name) code, #include "RuntimeLibcalls.def" #undef HANDLE_LIBCALL UNKNOWN_LIBCALL } and delete UNKNOWN_LIBCALL from RuntimeLibcalls.def. |
lib/Target/WebAssembly/WebAssemblyRuntimeLibcallSignatures.cpp | ||
---|---|---|
92–95 ↗ | (On Diff #107208) | Looks like all this should be: RuntimeLibcallSignatureTable() : Table(RTLIB::UNKNOWN_LIBCALL, unsupported) { |
95 ↗ | (On Diff #107208) | If this table is filled with 'unsupported' - could this do away with all the unsupported cases/initializations below? |
900 ↗ | (On Diff #107208) | Could this maybe be a static local in GetSignature instead? Seems like it'd be simpler - there's no need to destroy it with llvm_shutdown, etc. (I suppose arguably it'd reduce memory usage/free unused memory for a client that's finished using LLVM for now, etc) |
- remove redundant unsupported elements and simplify initialization
lib/Target/WebAssembly/WebAssemblyRuntimeLibcallSignatures.cpp | ||
---|---|---|
95 ↗ | (On Diff #107208) | Yes, it could. Most of the missing ones are for atomics, which I actually am planning on implementing soon. So I left those as TODO. The others I just removed. |
883 ↗ | (On Diff #107208) | UNKNOWN_LIBCALL has a bunch of uses as a sentinel value elsewhere, so I don't think we should remove it. Something like LAST_LIBCALL or similar might be more consistent with Instruction.def but that pattern doesn't seem universal, and it also subtly changes the meaning of the sentinel (i.e. LAST_INSTRUCTION is a valid inst). So I'm inclined to leave it. |
900 ↗ | (On Diff #107208) | Yeah, it could, and it would be a bit simpler. The motivation was to avoid constructing the big data structures (signature table and name map) unless the wasm backend was actually used. Also I wanted to avoid having a static std::map. Chrome has a pretty strong convention against static constructors, does LLVM have anything similar? |
Coming back to this patch, in order to remove the tight coupling between the arrays in WebAssemblyRuntimeLibcallSignatures.cpp and the list of libcalls in the def file.
In https://reviews.llvm.org/D35522 we discussed changing the API of TargetLoweringBase::getLibcallName() and I experimented with that but it ended up being a more pervasive change than I expected, and I wasn't sure the result was any better than what we have now. So I updated this PR, and I still like it for the reasons I mentioned in the comments before, but happy to take more input now.
Ping! This refactoring makes less work for anyone changing libcalls... any review takers?
lib/Target/WebAssembly/WebAssemblyRuntimeLibcallSignatures.cpp | ||
---|---|---|
900 ↗ | (On Diff #107208) | We do, but static locals are fine. They allow for lazy initialization at a small performance cost. Consider that errs() and outs() are implemented that way. |