[PATCH] Respect TargetLibraryInfo CustomNames in getLibFunc (was: Respect TargetLibraryInfo CustomNames in getAllocationData)
Shea Levy
shea at shealevy.com
Mon Feb 10 18:17:24 PST 2014
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;
> }
More information about the llvm-commits
mailing list