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

[Hal Finkel]

----- 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.

+        if (getState(F) == CustomName)
+            return true;

This condition should never be false, right? I think that an assert would be better.

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