[PATCH] D35522: Move Runtime libcall definitions to a .def file

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Couple of small things, but otherwise looks good.



================
Comment at: include/llvm/CodeGen/RuntimeLibcalls.def:29-31
+#ifndef HANDLE_LIBCALL
+#define HANDLE_LIBCALL(code, name)
+#endif
----------------
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.


================
Comment at: lib/CodeGen/TargetLoweringBase.cpp:100-110
   if (TT.isGNUEnvironment()) {
     Names[RTLIB::SINCOS_F32] = "sincosf";
     Names[RTLIB::SINCOS_F64] = "sincos";
     Names[RTLIB::SINCOS_F80] = "sincosl";
     Names[RTLIB::SINCOS_F128] = "sincosl";
     Names[RTLIB::SINCOS_PPCF128] = "sincosl";
   }
----------------
dschuff wrote:
> dschuff wrote:
> > dblaikie wrote:
> > > dblaikie wrote:
> > > > dschuff wrote:
> > > > > dschuff wrote:
> > > > > > dblaikie wrote:
> > > > > > > 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)?
> > > > > > 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.
> > > > > 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?
> > > > 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.
> > > Using empty string seems like a fine idea & seems worthwhile given how it'd simplify the next one.
> > 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.
> 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 
> 1. Most of its users don't explicitly handle the error/exceptional case at all (and it would probably be a bug if there were one); for those users nullptr will likely make it easier to debug those cases.
> 2. For the ones that do check, checking for nullptr still seems a bit nicer than checking for an emtpy C string.
> 
> 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.
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.


https://reviews.llvm.org/D35522