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

[Shea Levy]

Hi Hal,

On Mon, Feb 10, 2014 at 07:32:13PM -0600, Hal Finkel wrote:
> ----- Original Message -----
> > From: "Shea Levy" <shea at shealevy.com>
> > To: llvm-commits at cs.uiuc.edu
> > Cc: "Hal Finkel" <hfinkel at anl.gov>, "Nuno Lopes" <nunoplopes at sapo.pt>
> > Sent: Monday, February 10, 2014 2:55:17 PM
> > Subject: [PATCH] Respect TargetLibraryInfo CustomNames in getLibFunc (was: Respect TargetLibraryInfo CustomNames in
> > getAllocationData)
> > 
> > OK, I've attached a patch to fix getLibFunc. Thanks for the feedback!
> 
> First, in terms of the coding conventions:
> 
> +  for (DenseMap<unsigned, std::string>::const_iterator i = CustomNames.begin(); i != CustomNames.end(); ++i) {
> 
> this line is too long (80-character max).
> 
> However, generally, I find adding a linear scan over the CustomNames map somewhat disturbing. I think that it would be better to add a second map <std::string, unsigned>, and use a lookup in that map instead. I'd rather take the extra small memory overhead.
> 

OK, attached patch uses a std::map<std::string, unsigned>

>
> +        if (getState(F) == CustomName)
> +            return true;
> 
> This condition should never be false, right? I think that an assert would be better.
> 

There's nothing stopping a user from calling setAvailableWithName then
later calling setUnavailable, is there?

Thanks,
Shea

>
> Thanks again,
> Hal
> 
> > 
> > ~Shea
> > 
> > On Mon, Feb 10, 2014 at 07:51:47PM -0000, Nuno Lopes wrote:
> > > >>Hi all,
> > > >>
> > > >>Currently, getAllocationData in MemoryBuiltins.cpp (a helper used
> > > >>for
> > > >>functions like isAllocLikeFn) uses TargetLibraryInfo::getLibFunc
> > > >>on
> > > >>the
> > > >>passed in function name to determine if it corresponds to a
> > > >>LibFunc::Func. Unfortunately, getLibFunc looks up Funcs by
> > > >>StandardName
> > > >>(e.g. "malloc" -> LibFunc::Func::malloc) and doesn't respect any
> > > >>overridden names set by TargetLibraryInfo::setAvailableWithName.
> > > >>This
> > > >>is
> > > >>perhaps a bug in getLibFunc (if it isn't, getLibFunc should
> > > >>probably
> > > >>be
> > > >>static), but since existing code may depend on this behavior
> > > >
> > > >Someone else may disagree ;) -- but I see no reason to do it this
> > > >way. If
> > > >the current code ignores name substitutions made using
> > > >setAvailableWithName, then it is broken, and should be fixed.
> > > >Please
> > > >submit a patch to properly fix the code in getLibFunc, thanks!
> > > 
> > > I second that. I did a quick grep for getLibFunc() usage and it
> > > seems that
> > > the desired behavior is the proper one that you describe.
> > > So, I agree with Hal in that it should be getLibFunc() that should
> > > be fixed.
> > > 
> > > Thanks,
> > > Nuno
> > > 
> > > 
> > > >-Hal
> > > >
> > > >>the
> > > >>attached patch instead modifies getAllocationData to compare the
> > > >>name
> > > >>associated with each of the Funcs in AllocationFnData with the
> > > >>passed-in
> > > >>function names, and if it matches (which will by the way never be
> > > >>true
> > > >>if the Func is not enabled for that TLI) then that is the Func
> > > >>used
> > > >>for
> > > >>the rest of the function.
> > > >>
> > > >>My test case for this was compiling some IR that had a
> > > >>superfluous
> > > >>call
> > > >>to GC_malloc (return value unused) using a PassManagerBuilder at
> > > >>-O3.
> > > >>By
> > > >>default, the call wasn't optimized away (as expected), but when I
> > > >>set
> > > >>the builder's LibraryInfo to one with Func::malloc set to
> > > >>"GC_malloc",
> > > >>it was optimized away only with this patch applied.
> > > >>
> > > >>Cheers,
> > > >>Shea Levy
> > > >>
> > > >>P.S. I am not currently subscribed to the list, so please CC me
> > > >>in
> > > >>replies (though I'll check the list archives periodically for
> > > >>replies
> > > >>as
> > > >>well)
> > > 
> > 
> 
> -- 
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
-------------- next part --------------
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;
   static const char* StandardNames[LibFunc::NumLibFuncs];
 
   enum AvailabilityState {
@@ -763,7 +765,9 @@
     if (StandardNames[F] != Name) {
       setState(F, CustomName);
       CustomNames[F] = Name;
+      ReversedCustomNames[Name] = F;
       assert(CustomNames.find(F) != CustomNames.end());
+      assert(ReversedCustomNames.find(Name) != ReversedCustomNames.end());
     } 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);
+  if (i != ReversedCustomNames.end()) {
+      F = (LibFunc::Func) i->second;
+      if (getState(F) == CustomName)
+          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;
   }
   return false;
 }