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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked 2 inline comments as done.
tejohnson added a comment.

Thanks for the comments. I'm out for a few days, but will address these when I get back.



================
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)) {
----------------
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.


================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2373
+        // so use default nullptr for that optional parameter.
+        Constant *New = ConstantFoldConstant(C, DL);
         if (New && New != C)
----------------
gchatelet wrote:
> Maybe explicitly write the `nullptr`
will do


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66428