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

[Duncan P. N. Exon Smith]

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;
> }