This will allow eliminating the duplication of the names, and allow adding
extra information such as signatures in a future commit.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
This looks good to me -- very clean. I'm quite new to LLVM, so I don't feel comfortable giving this a go-ahead; I'll leave that to someone more experienced.
How would this be used in the WebAssembly backend? Would the idea be to expand the def file to provide the required WebAssembly values in each HANDLE_LIBCALL macro? Or change the static arrays to helper functions with a giant switch-case? Or... ?
Comment Actions
Yes; the wasm backend currently has a partial duplication of the libcall names that can go away (in fact, maybe i'll even roll that into this patch), and it also has signature information for each function. The signatures could go into the .def file as well, and the other users of the def file could just define a HANDLE_LIBCALL that ignores them.
| lib/CodeGen/TargetLoweringBase.cpp | ||
|---|---|---|
| 100–110 ↗ | (On Diff #106991) | Any reason these two are implemented differently (one has null in the table and initializer the values conditionally - the other has a value in the table and conditionally nulls it out here)? |
| lib/CodeGen/TargetLoweringBase.cpp | ||
|---|---|---|
| 100–110 ↗ | (On Diff #106991) | The idea was sort of that the default value in the table is the "normal" or most common value, and the exceptional cases are handled here. |
| lib/CodeGen/TargetLoweringBase.cpp | ||
|---|---|---|
| 100–110 ↗ | (On Diff #106991) | Another somewhat-related note, there ended up being a slight bit of ugliness (see line 883 of D35592) related to using nullptr here instead of e.g. an empty string. But that maybe is still better than using empty string instead of nullptr here? |
| lib/CodeGen/TargetLoweringBase.cpp | ||
|---|---|---|
| 100–110 ↗ | (On Diff #106991) | I'd /probably/ keep it one way or the other rather than picking/judging which case is more common/'normal'. But no big deal either way/whatever you prefer. |
| 100–110 ↗ | (On Diff #106991) | Using empty string seems like a fine idea & seems worthwhile given how it'd simplify the next one. |
| lib/CodeGen/TargetLoweringBase.cpp | ||
|---|---|---|
| 100–110 ↗ | (On Diff #106991) | There wasn't really any judgement involved. In all 3 cases here, there is one particular OS or environment that is the exception. I actually like the open code better this way anyway because the if conditions all go the same direction rather than one of them being inverted. I did clarify the comments here and the def file though. |
| 100–110 ↗ | (On Diff #106991) | I just looked at the other uses of TargetLoweringBase::getLibcallName and now I actually think that it is better if it returns nullptr instead of an empty string, because
For my use, the weirdness just comes from the fact that the macro expansion results in an explicit construction from nullptr, which doesn't work because I'm constructing a StringRef for a StringMap. A slightly heavier-weight solution could be to have getLibcallName return a StringRef (or even an Expected<StringRef>) instead of a char *. Many of the users are just immediately constructing a StringRef out of it anyway. |
Comment Actions
Couple of small things, but otherwise looks good.
| include/llvm/CodeGen/RuntimeLibcalls.def | ||
|---|---|---|
| 29–31 ↗ | (On Diff #107337) | This should probably be an error (to cite an example: include/llvm/BinaryFormat/ELFRelocs/x86_64.def). .def files with more than one macro would have this kind of silencing/empty macro definition - but with only one macro an error if that macro is not defined seems good. |
| lib/CodeGen/TargetLoweringBase.cpp | ||
| 100–110 ↗ | (On Diff #106991) | Expected<StringRef> is probably the nicest, if you want to do that - but if const char* isn't used much/doesn't leak out into lots of other APIs no big deal either way. |
| lib/CodeGen/TargetLoweringBase.cpp | ||
|---|---|---|
| 100–110 ↗ | (On Diff #106991) | Cool, I'll look into Expected<StringRef> and see what happens. |
Comment Actions
So it looks like switching the API away from char*-based would be, maybe not super-trivial, but probably not too bad. It's interlinked essentially with other APIs relating to external symbols:
mostly SelectionDAG::getExternalSymbol(), and also MachineInstrBuilder::addExternalSymbol() but those could be converted too, and I don't think I'd mind doing it as general cleanup. I wonder if Expected<StringRef> is a little heavyweight though. It seems to have a really strong convention that the result should be checked first, and most use cases just don't care (arguably they could assert, but requiring that would add a lot of boilerplate asserts for probably not a lot of benefit). StringRef by itself (with empty string taking the place of the current use of nullptr) wouldn't be too bad, although having the empty string as a sort of sentinel value only used by a small fraction of the uses doesn't seem ideal either. I might still be inclined to prefer that over Expected though. Any thoughts?
Comment Actions
Actually, maybe an even better idea:
getLibcallName() returns a StringRef and asserts rather than returning a failure code. getLIbcallNameIfAvailable() returns an Expected<StringRef> and can fail.
The former matches 90% of the existing uses. This would let us move away from char* for the external symbol APIs (assuming that's a good thing) but most of the existing uses wouldn't need to actually change, and those that check failure would still be simple.