[PATCH] D124529: [CompileTime] [Passes] Avoid computing unnecessary analyses. NFC

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp:663
+    if (LI.empty())
+      return false;
     auto &LAA = getAnalysis<LoopAccessLegacyAnalysis>();
----------------
nikic wrote:
> This doesn't really make sense for the legacy pass manager, because it will always calculate the analysis, independently of whether it is actually accessed. I'd suggest leaving the LegacyPM implementation alone.
okay agreed. I didn't bother with legacy PM for the other passes, so will remove from this one as well.


================
Comment at: llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp:713
                                                FunctionAnalysisManager &AM) {
-  auto &SE = AM.getResult<ScalarEvolutionAnalysis>(F);
+  auto &DT = AM.getResult<DominatorTreeAnalysis>(F);
   auto &LI = AM.getResult<LoopAnalysis>(F);
----------------
nikic wrote:
> I would keep the DT fetch below the LI.empty() check as well. Yes, it's not going to matter for compile-time because LI computes DT anyway, but there's also no need to specially highlight DT here. Same below.
Hmm. I actually kept it above to show exactly that DT will need to be computed as part of LI computation. Otherwise the comment at line 715 seems misleading "Return before computing other expensive analyses."

Although one can say that if the other analyses were already cached, we won't be computing them anyway here...
Maybe change it to `AM.getCachedResult<DominatorTreeAnalysis>(F)` if the check if moved after the `LI.empty()` check?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124529