[PATCH] D66428: Change TargetLibraryInfo analysis passes to always require Function

[Guillaume Chatelet via Phabricator via llvm-commits]

gchatelet marked an inline comment as done.
gchatelet added inline comments.


================
Comment at: lib/Analysis/GlobalsModRef.cpp:580
         if (auto *Call = dyn_cast<CallBase>(&I)) {
+          auto TLI = GetTLI(*Node->getFunction());
           if (isAllocationFn(Call, &TLI) || isFreeCall(Call, &TLI)) {
----------------
tejohnson wrote:
> gchatelet wrote:
> > Would `auto&` be more appropriate here? I'm concerned about a possible copy - we're taking the address of `TLI` on next line.
> > Same in the rest of the code.
> It should be a cheap copy (TargetLibraryInfo only contains a pointer to its implementation), but yes we should be able to do this. I had run into issues earlier in another file due to the lifetime of a temporary returned by the lambda ending before the uses, but it looks like that can be addressed by changing the lambda there to have an explicit return type that is a reference, so that it doesn't return a temporary. I'll give that a try.
Looks better now IMHO. Thx!


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66428/new/

https://reviews.llvm.org/D66428