[PATCH] D35592: [WebAssembly] Remove duplicated RTLIB names

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyRuntimeLibcallSignatures.cpp:95
+    // Any newly-added libcalls will be unsupported by default.
+    std::fill(Table.begin(), Table.end(), unsupported);
+
----------------
dblaikie wrote:
> If this table is filled with 'unsupported' - could this do away with all the unsupported cases/initializations below?
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.


================
Comment at: lib/Target/WebAssembly/WebAssemblyRuntimeLibcallSignatures.cpp:883
 
+static StringRef StringRefOrEmpty(const char* arg) {
+  if (arg) return StringRef(arg);
----------------
aheejin wrote:
> dschuff wrote:
> > Can't construct a StringRef will nullptr, so we have this bit of ugliness.
> 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.
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.


================
Comment at: lib/Target/WebAssembly/WebAssemblyRuntimeLibcallSignatures.cpp:900
+
+static ManagedStatic<StaticLibcallNameMap> LibcallNameMap;
+// TODO: If the RTLIB::Libcall-taking flavor of GetSignature remains unsed
----------------
dblaikie wrote:
> 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)
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?


https://reviews.llvm.org/D35592