[PATCH] Respect TargetLibraryInfo CustomNames in getLibFunc (was: Respect TargetLibraryInfo CustomNames in getAllocationData)

[Shea Levy]

Hi Duncan,

Thanks for the review. I won't be able to get to it for a few days but
I'll try to get a revised version out later this week.

~Shea

On Sat, Mar 01, 2014 at 07:06:45PM -0800, Duncan P. N. Exon Smith wrote:
> On 2014 Feb 26, at 17:33, Shea Levy <shea at shealevy.com> wrote:
> 
> > Ping?
> > 
> > CCing sabre who is listed as the owner of "Everything not covered by
> > someone else", as it's not obvious to me who owns TargetLibraryInfo.
> 
> I have some comments below.  Firstly, though, where’s the testcase?
> 
> > Index: include/llvm/Target/TargetLibraryInfo.h
> > ===================================================================
> > --- include/llvm/Target/TargetLibraryInfo.h	(revision 201032)
> > +++ include/llvm/Target/TargetLibraryInfo.h	(working copy)
> > @@ -12,6 +12,7 @@
> >  
> >  #include "llvm/ADT/DenseMap.h"
> >  #include "llvm/Pass.h"
> > +#include <map>
> >  
> >  namespace llvm {
> >    class Triple;
> > @@ -676,6 +677,7 @@
> >    virtual void anchor();
> >    unsigned char AvailableArray[(LibFunc::NumLibFuncs+3)/4];
> >    llvm::DenseMap<unsigned, std::string> CustomNames;
> > +  std::map<std::string, unsigned> ReversedCustomNames;
> 
> Use a StringMap<unsigned> here instead of a std::map.
> 
> >    static const char* StandardNames[LibFunc::NumLibFuncs];
> >  
> >    enum AvailabilityState {
> > @@ -762,8 +764,17 @@
> >    void setAvailableWithName(LibFunc::Func F, StringRef Name) {
> >      if (StandardNames[F] != Name) {
> >        setState(F, CustomName);
> > -      CustomNames[F] = Name;
> > +      llvm::DenseMap<unsigned, std::string>::iterator i =
> 
> Variable names should be capitalized.  Use I instead of i.
> 
> > +          CustomNames.find(F);
> > +      if (i != CustomNames.end()) {
> > +        ReversedCustomNames.erase(i->second);
> > +        i->second = Name;
> > +        CustomNames.insert(*i);
> 
> What's this CustomNames.insert() call for?  Looks like a no-op.
> 
> > +      } else
> > +        CustomNames[F] = Name;
> > +      ReversedCustomNames[Name] = F;
> >        assert(CustomNames.find(F) != CustomNames.end());
> > +      assert(ReversedCustomNames.find(Name) != ReversedCustomNames.end());
> 
> This is overly verbose.  I'd update it to:
> 
>     assert(ReversedCustumNames.count(Name));
> 
> You may as well update the other asseriton, as well.
> 
> >      } else {
> >        setState(F, StandardName);
> >      }
> > Index: lib/Target/TargetLibraryInfo.cpp
> > ===================================================================
> > --- lib/Target/TargetLibraryInfo.cpp	(revision 201032)
> > +++ lib/Target/TargetLibraryInfo.cpp	(working copy)
> > @@ -684,6 +684,7 @@
> >    : ImmutablePass(ID) {
> >    memcpy(AvailableArray, TLI.AvailableArray, sizeof(AvailableArray));
> >    CustomNames = TLI.CustomNames;
> > +  ReversedCustomNames = TLI.ReversedCustomNames;
> >  }
> >  
> >  namespace {
> > @@ -707,9 +708,6 @@
> >  
> >  bool TargetLibraryInfo::getLibFunc(StringRef funcName,
> >                                     LibFunc::Func &F) const {
> > -  const char **Start = &StandardNames[0];
> > -  const char **End = &StandardNames[LibFunc::NumLibFuncs];
> > -
> >    // Filter out empty names and names containing null bytes, those can't be in
> >    // our table.
> >    if (funcName.empty() || funcName.find('\0') != StringRef::npos)
> > @@ -719,10 +717,25 @@
> >    // strip it if present.
> >    if (funcName.front() == '\01')
> >      funcName = funcName.substr(1);
> > +
> > +  // First see if funcName is in CustomNames
> > +  std::map<std::string, unsigned>::const_iterator i =
> > +      ReversedCustomNames.find(funcName);
> 
> Again, use I instead of i.
> 
> > +  if (i != ReversedCustomNames.end()) {
> > +      F = (LibFunc::Func) i->second;
> 
> Too much indentation here.  Should be two spaces for indented blocks.
> 
> > +      if (getState(F) == CustomName)
> > +          return true;
> 
> This flow isn't quite right.  You've already set F, but now might not return
> true.  The header docs for getLibFunc say that F changes iff the library
> function is found.
> 
> I think this makes more sense:
> 
>     if (I != ReversedCustomNames.end())
>       if (getState((LibFunc::Func)I->second) == CustomName) {
>         F = (LibFunc::Func)I->second;
>         return true;
>       }
> 
> > +  }
> > +
> > +  // Now check if it's a StandardName
> > +  const char **Start = &StandardNames[0];
> > +  const char **End = &StandardNames[LibFunc::NumLibFuncs];
> > +
> >    const char **I = std::lower_bound(Start, End, funcName, StringComparator());
> >    if (I != End && *I == funcName) {
> >      F = (LibFunc::Func)(I - Start);
> > -    return true;
> > +    if (getState(F) == StandardName)
> > +        return true;
> 
> Same comment here.  Also, watch the indentation.
> 
> >   }
> >   return false;
> > }
>