[PATCH] Respect TargetLibraryInfo CustomNames in getLibFunc (was: Respect TargetLibraryInfo CustomNames in getAllocationData)
Shea Levy
shea at shealevy.com
Tue Feb 11 05:55:56 PST 2014
OK, attached patch removes old names if a new name is set.
On Mon, Feb 10, 2014 at 09:17:24PM -0500, Shea Levy wrote:
> Ah, on second thought that patch doesn't remove old names if a new name
> is set. Will try something new tomorrow.
>
> On Mon, Feb 10, 2014 at 09:14:46PM -0500, Shea Levy wrote:
> > 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
>
> > 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;
> > }
>
-------------- 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 {
@@ -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 =
+ CustomNames.find(F);
+ if (i != CustomNames.end()) {
+ ReversedCustomNames.erase(i->second);
+ i->second = Name;
+ CustomNames.insert(*i);
+ } else
+ 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;
}
More information about the llvm-commits
mailing list