[PATCH] Respect TargetLibraryInfo CustomNames in getLibFunc (was: Respect TargetLibraryInfo CustomNames in getAllocationData)
Shea Levy
shea at shealevy.com
Wed Feb 26 17:33:54 PST 2014
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.
On Tue, Feb 11, 2014 at 08:55:56AM -0500, Shea Levy wrote:
> 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;
> > > }
> >
> 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