[PATCH] D77952: [TLII] Reduce copies of TLII for TLA

[Wenlei He via Phabricator via cfe-commits]

wenlei marked 2 inline comments as done.
wenlei added inline comments.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:689
   // Set up the per-function pass manager.
-  FPM.add(new TargetLibraryInfoWrapperPass(*TLII));
+  FPM.add(new TargetLibraryInfoWrapperPass(TargetTriple));
   if (CodeGenOpts.VerifyModule)
----------------
tejohnson wrote:
> wenlei wrote:
> > tejohnson wrote:
> > > These changes mean we now construct a new TLII multiple times (e.g. both when we add the TargetLibraryInfoWrapperPass to the MPM earlier and to the FPM here, rather that just copying. Is this actually faster? It seems like it would be slower overall.
> > Oops, this one isn't intentional... changed it back. Though for other instances where TLII isn't reused, similar change turns extra copy into move.
> I suppose you could std::move the TLII here to avoid this second copy.
> 
> Do you know how much difference this patch makes on the compile time instruction count regressions seen with the original patch? It seems like it shouldn't be huge given that this is just during the pipeline setup for the module. But if it does explain the instruction count increases that's probably why it didn't regress the actual wall time measurements.
Yeah, the 2nd use can be a move, though I'm inclined to not do that, as TLII isn't immediately out of scope yet.

I didn't setup measurement for this. I was basically playing with some tests for sanity check just to make sure we're not doing things unexpectedly, e.g. we don't do per-function copies unexpectedly in `TargetLibraryAnalysis::run`. But other than a few extra copies on setup path, I didn't see anything unusual. Since the original patch can make any copies more expensive, I thought it's good to reduce that as I see it.


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:596
   memcpy(AvailableArray, TLI.AvailableArray, sizeof(AvailableArray));
   return *this;
 }
----------------
tejohnson wrote:
> This is missing copying of the VecLibDescs (looks like I missed this as well originally).
Now I remembered why this was missed from my side, before my patch, `VecLibDescs` isn't copied here either, which seems like a bug? Same for the move one below. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77952