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

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
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";
   }
----------------
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.


================
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:
> 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.


https://reviews.llvm.org/D35522