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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
