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

[Shea Levy]

OK, I've attached a patch to fix getLibFunc. Thanks for the feedback!

~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)
> 
-------------- next part --------------
Index: lib/Target/TargetLibraryInfo.cpp
===================================================================
--- lib/Target/TargetLibraryInfo.cpp	(revision 201032)
+++ lib/Target/TargetLibraryInfo.cpp	(working copy)
@@ -707,9 +707,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 +716,25 @@
   // strip it if present.
   if (funcName.front() == '\01')
     funcName = funcName.substr(1);
+
+  // First see if funcName is in CustomNames
+  for (DenseMap<unsigned, std::string>::const_iterator i = CustomNames.begin(); i != CustomNames.end(); ++i) {
+    if (funcName == i->second) {
+        F = (LibFunc::Func) i->first;
+        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;
 }