This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Remove duplicated RTLIB names
ClosedPublic

Authored by dschuff on Jul 18 2017, 4:54 PM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

dschuff created this revision.Jul 18 2017, 4:54 PM
dschuff added inline comments.
lib/Target/WebAssembly/WebAssemblyRuntimeLibcallSignatures.cpp
883 ↗(On Diff #107208)

Can't construct a StringRef will nullptr, so we have this bit of ugliness.

aheejin added inline comments.Jul 18 2017, 6:28 PM
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.

dblaikie added inline comments.Jul 19 2017, 8:25 AM
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)

dschuff updated this revision to Diff 107339.Jul 19 2017, 10:32 AM
dschuff marked an inline comment as done.
  • 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?

aheejin accepted this revision.Jul 20 2017, 1:44 PM
This revision is now accepted and ready to land.Jul 20 2017, 1:44 PM

Just saw this hasn't been landed. Are you waiting from @dblaikie ?

dschuff updated this revision to Diff 129158.Jan 9 2018, 2:22 PM
  • Merge branch 'master' into wasm_rtlib_def
dschuff updated this revision to Diff 129162.Jan 9 2018, 2:38 PM
  • fix conflicts

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.

dschuff updated this revision to Diff 130297.Jan 17 2018, 3:29 PM
  • Merge branch 'master' into wasm_rtlib_def
  • fix conflict

Ping! This refactoring makes less work for anyone changing libcalls... any review takers?

sunfish accepted this revision.Jan 17 2018, 3:40 PM

Looks good to me. Thanks for cleaning this up!

lib/Target/WebAssembly/WebAssemblyRuntimeLibcallSignatures.cpp
90 ↗(On Diff #130297)

This struct should be defined in an anonymous namespace.

787 ↗(On Diff #130297)

This struct should be defined in an anonymous namespace.

rnk added inline comments.Jan 17 2018, 4:31 PM
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.

dschuff updated this revision to Diff 130330.Jan 17 2018, 5:09 PM
  • move types into anon namespace (and static stuff too just for consistency
This revision was automatically updated to reflect the committed changes.
dschuff marked 2 inline comments as done.