Clean up strange uses of getAnalysisIfAvailable #65729

jayfoad · 2023-09-08T09:12:11Z

After a pass calls addRequired() it is strange to call
getAnalysisIfAvailable() because analysis X should always be
available. Use getAnalysis() instead.

After a pass calls addRequired<X>() it is strange to call getAnalysisIfAvailable<X>() because analysis X should always be available. Use getAnalysis<X>() instead.

nikic

LGTM

GVN seems to have different behavior regarding LoopInfo in the LegacyPM and NewPM, but that's an orthogonal issue. I'll check whether always requiring LoopInfo in GVN has any compile-time impact, as I don't think this is supposed to be an optional analysis there (otherwise loop load PRE is not going to work reliably).

llvmbot · 2023-10-11T07:26:38Z

@llvm/pr-subscribers-llvm-transforms

Author: Jay Foad (jayfoad)

Changes

After a pass calls addRequired<X>() it is strange to call
getAnalysisIfAvailable<X>() because analysis X should always be
available. Use getAnalysis<X>() instead.

Full diff: https://github.com/llvm/llvm-project/pull/65729.diff

5 Files Affected:

(modified) llvm/lib/CodeGen/EarlyIfConversion.cpp (+2-2)
(modified) llvm/lib/CodeGen/TypePromotion.cpp (+2-5)
(modified) llvm/lib/CodeGen/VirtRegMap.cpp (+1-1)
(modified) llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp (+1-1)
(modified) llvm/lib/Transforms/Scalar/GVN.cpp (+2-2)

diff --git a/llvm/lib/CodeGen/EarlyIfConversion.cpp b/llvm/lib/CodeGen/EarlyIfConversion.cpp
index 61867d74bfa293c..3086d953a23699f 100644
--- a/llvm/lib/CodeGen/EarlyIfConversion.cpp
+++ b/llvm/lib/CodeGen/EarlyIfConversion.cpp
@@ -1092,7 +1092,7 @@ bool EarlyIfConverter::runOnMachineFunction(MachineFunction &MF) {
   SchedModel = STI.getSchedModel();
   MRI = &MF.getRegInfo();
   DomTree = &getAnalysis<MachineDominatorTree>();
-  Loops = getAnalysisIfAvailable<MachineLoopInfo>();
+  Loops = &getAnalysis<MachineLoopInfo>();
   Traces = &getAnalysis<MachineTraceMetrics>();
   MinInstr = nullptr;
 
@@ -1226,7 +1226,7 @@ bool EarlyIfPredicator::runOnMachineFunction(MachineFunction &MF) {
   MRI = &MF.getRegInfo();
   SchedModel.init(&STI);
   DomTree = &getAnalysis<MachineDominatorTree>();
-  Loops = getAnalysisIfAvailable<MachineLoopInfo>();
+  Loops = &getAnalysis<MachineLoopInfo>();
   MBPI = &getAnalysis<MachineBranchProbabilityInfo>();
 
   bool Changed = false;
diff --git a/llvm/lib/CodeGen/TypePromotion.cpp b/llvm/lib/CodeGen/TypePromotion.cpp
index 51f77e5fd8b08af..053caf518bd1f78 100644
--- a/llvm/lib/CodeGen/TypePromotion.cpp
+++ b/llvm/lib/CodeGen/TypePromotion.cpp
@@ -1016,11 +1016,8 @@ bool TypePromotionLegacy::runOnFunction(Function &F) {
   if (skipFunction(F))
     return false;
 
-  auto *TPC = getAnalysisIfAvailable<TargetPassConfig>();
-  if (!TPC)
-    return false;
-
-  auto *TM = &TPC->getTM<TargetMachine>();
+  auto &TPC = getAnalysis<TargetPassConfig>();
+  auto *TM = &TPC.getTM<TargetMachine>();
   auto &TTI = getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F);
   auto &LI = getAnalysis<LoopInfoWrapperPass>().getLoopInfo();
 
diff --git a/llvm/lib/CodeGen/VirtRegMap.cpp b/llvm/lib/CodeGen/VirtRegMap.cpp
index c1c6ce227334a4b..6e29a41b617a87e 100644
--- a/llvm/lib/CodeGen/VirtRegMap.cpp
+++ b/llvm/lib/CodeGen/VirtRegMap.cpp
@@ -261,7 +261,7 @@ bool VirtRegRewriter::runOnMachineFunction(MachineFunction &fn) {
   Indexes = &getAnalysis<SlotIndexes>();
   LIS = &getAnalysis<LiveIntervals>();
   VRM = &getAnalysis<VirtRegMap>();
-  DebugVars = getAnalysisIfAvailable<LiveDebugVariables>();
+  DebugVars = &getAnalysis<LiveDebugVariables>();
   LLVM_DEBUG(dbgs() << "********** REWRITE VIRTUAL REGISTERS **********\n"
                     << "********** Function: " << MF->getName() << '\n');
   LLVM_DEBUG(VRM->dump());
diff --git a/llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp b/llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp
index 4c8c03a4c693fe1..ad0ff8335647419 100644
--- a/llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp
@@ -935,7 +935,7 @@ bool AArch64ConditionalCompares::runOnMachineFunction(MachineFunction &MF) {
   SchedModel = MF.getSubtarget().getSchedModel();
   MRI = &MF.getRegInfo();
   DomTree = &getAnalysis<MachineDominatorTree>();
-  Loops = getAnalysisIfAvailable<MachineLoopInfo>();
+  Loops = &getAnalysis<MachineLoopInfo>();
   MBPI = &getAnalysis<MachineBranchProbabilityInfo>();
   Traces = &getAnalysis<MachineTraceMetrics>();
   MinInstr = nullptr;
diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
index 4c5b0ff3af16e53..f419d03c0ce3837 100644
--- a/llvm/lib/Transforms/Scalar/GVN.cpp
+++ b/llvm/lib/Transforms/Scalar/GVN.cpp
@@ -3286,7 +3286,7 @@ class llvm::gvn::GVNLegacyPass : public FunctionPass {
     if (skipFunction(F))
       return false;
 
-    auto *LIWP = getAnalysisIfAvailable<LoopInfoWrapperPass>();
+    auto &LIWP = getAnalysis<LoopInfoWrapperPass>();
 
     auto *MSSAWP = getAnalysisIfAvailable<MemorySSAWrapperPass>();
     return Impl.runImpl(
@@ -3297,7 +3297,7 @@ class llvm::gvn::GVNLegacyPass : public FunctionPass {
         Impl.isMemDepEnabled()
             ? &getAnalysis<MemoryDependenceWrapperPass>().getMemDep()
             : nullptr,
-        LIWP ? &LIWP->getLoopInfo() : nullptr,
+        &LIWP.getLoopInfo(),
         &getAnalysis<OptimizationRemarkEmitterWrapperPass>().getORE(),
         MSSAWP ? &MSSAWP->getMSSA() : nullptr);
   }

bjope

I guess I don't know how pull requests and reviewing works in github. I actually added 3 comments on this patch a several days (or weeks) ago. But turns out that they were "pending" because I had only "started review" and not found the place to "submit review".

So sorry for some late comments here. Since this has been pushed already, then I guess you may ignore them.

bjope · 2023-09-16T10:50:02Z

llvm/lib/CodeGen/VirtRegMap.cpp

@@ -261,7 +261,7 @@ bool VirtRegRewriter::runOnMachineFunction(MachineFunction &fn) {
  Indexes = &getAnalysis<SlotIndexes>();
  LIS = &getAnalysis<LiveIntervals>();
  VRM = &getAnalysis<VirtRegMap>();
-  DebugVars = getAnalysisIfAvailable<LiveDebugVariables>();
+  DebugVars = &getAnalysis<LiveDebugVariables>();


Not sure how to add a comment on line 278 if I want to suggest an edit on that line (in Phabricator it would have been easy). But I guess the check if DebugVars is null or not on line 278 is redundant now.

Thanks! Fixed in 05c16f4

bjope · 2023-09-16T11:38:50Z

llvm/lib/Target/AArch64/AArch64ConditionalCompares.cpp

@@ -935,7 +935,7 @@ bool AArch64ConditionalCompares::runOnMachineFunction(MachineFunction &MF) {
  SchedModel = MF.getSubtarget().getSchedModel();
  MRI = &MF.getRegInfo();
  DomTree = &getAnalysis<MachineDominatorTree>();
-  Loops = getAnalysisIfAvailable<MachineLoopInfo>();
+  Loops = &getAnalysis<MachineLoopInfo>();


I think there are some "weird" things going on here. But maybe not worth digging into those unless being an AArch64 maintainer.

Afaict the goal here is to make sure MachineLoopInfo is preserved. So this pass shouldn't really need to require it explicitly. However, the MachineTraceMetrics pass is also using MachineLoopInfo transitively. So, the MachineLoopInfo analysis will be run. Although I think there is a "bug" in MachineTraceMetrics related to not using addRequiredTransitive. So things might break if removing the addRequired in this pass.

To sum up. The proposed change is probably harmless and OK. Just saying that the analysis pass dependencies used in here seem to be a bit ad-hoc(?).

Everything seems to work if I change AArch64ConditionalCompares to not require MachineLoopInfo. But I think I'll leave it to the AArch64 maintainers like you suggest.

bjope · 2023-09-16T11:38:52Z

llvm/lib/Transforms/Scalar/GVN.cpp

@@ -3280,7 +3280,7 @@ class llvm::gvn::GVNLegacyPass : public FunctionPass {
    if (skipFunction(F))
      return false;

-    auto *LIWP = getAnalysisIfAvailable<LoopInfoWrapperPass>();
+    auto &LIWP = getAnalysis<LoopInfoWrapperPass>();


IIUC GVN doesn't depend on LoopInfo itself. It should just make sure to preserve it if it is available. So one might wonder if it is the addRequire that should be questioned and possibly removed instead.
But it's been like this for years (that it is required), and messing around with the legacy PM analysis dependencies for an IR pass (even if it is used by some backends) is perhaps not worth it.

Therefore I think you proposed change is ok. Or you could just leave this as is, since the IfAvailable part in some sense indicate that the pass itself doesn't depend on the analysis.

I prefer not to leave it as is - I think either we should get the analysis with getAnalysis or we should remove the addRequired, so that they are consistent with each other.

jayfoad · 2023-10-11T09:30:52Z

I guess I don't know how pull requests and reviewing works in github. I actually added 3 comments on this patch a several days (or weeks) ago. But turns out that they were "pending" because I had only "started review" and not found the place to "submit review".

For that reason I usually click "add single comment" instead of "start a review".

nikic · 2023-10-11T09:33:47Z

GVN seems to have different behavior regarding LoopInfo in the LegacyPM and NewPM, but that's an orthogonal issue. I'll check whether always requiring LoopInfo in GVN has any compile-time impact, as I don't think this is supposed to be an optional analysis there (otherwise loop load PRE is not going to work reliably).

I've checked, and LoopInfo is always available in pipeline positions where GVN runs, so this effectively has no impact. I've made it a required analysis for the NewPM as well in fa58847.

bjope · 2023-10-12T08:03:15Z

I guess I don't know how pull requests and reviewing works in github. I actually added 3 comments on this patch a several days (or weeks) ago. But turns out that they were "pending" because I had only "started review" and not found the place to "submit review".

For that reason I usually click "add single comment" instead of "start a review".

Thanks. I'll try to remember that.

If they for example had named the buttons "Post comment directly" and "Save as draft", then it has been more obvious.
I also do not know how to find all my pending "draft reviews" (unless I'm already "assigned" or "requested as reviewer") as the https://github.com/pulls is just crap compared to my old dashboard in phabricator.

Clean up strange uses of getAnalysisIfAvailable

66129ac

After a pass calls addRequired<X>() it is strange to call getAnalysisIfAvailable<X>() because analysis X should always be available. Use getAnalysis<X>() instead.

jayfoad requested review from a team as code owners September 8, 2023 09:12

github-actions bot added the backend:AArch64 label Sep 8, 2023

dtcxzyw requested review from nikic and removed request for a team October 11, 2023 06:42

nikic approved these changes Oct 11, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into getanalysisifavailable

0fec568

llvmbot added the llvm:transforms label Oct 11, 2023

jayfoad merged commit b78f3ea into llvm:main Oct 11, 2023

bjope reviewed Oct 11, 2023

View reviewed changes

jayfoad deleted the getanalysisifavailable branch October 11, 2023 09:36

jayfoad added a commit that referenced this pull request Oct 11, 2023

[VirtRegMap] Simplify condition after #65729

05c16f4

jayfoad added a commit that referenced this pull request Oct 11, 2023

[EarlyIfConversion] Simplify condition after #65729

fac4206

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up strange uses of getAnalysisIfAvailable #65729

Clean up strange uses of getAnalysisIfAvailable #65729

jayfoad commented Sep 8, 2023

nikic left a comment

llvmbot commented Oct 11, 2023

bjope left a comment

bjope Sep 16, 2023

jayfoad Oct 11, 2023

bjope Sep 16, 2023

jayfoad Oct 11, 2023

bjope Sep 16, 2023

jayfoad Oct 11, 2023

jayfoad commented Oct 11, 2023

nikic commented Oct 11, 2023

bjope commented Oct 12, 2023

Clean up strange uses of getAnalysisIfAvailable #65729

Clean up strange uses of getAnalysisIfAvailable #65729

Conversation

jayfoad commented Sep 8, 2023

nikic left a comment

Choose a reason for hiding this comment

llvmbot commented Oct 11, 2023

bjope left a comment

Choose a reason for hiding this comment

bjope Sep 16, 2023

Choose a reason for hiding this comment

jayfoad Oct 11, 2023

Choose a reason for hiding this comment

bjope Sep 16, 2023

Choose a reason for hiding this comment

jayfoad Oct 11, 2023

Choose a reason for hiding this comment

bjope Sep 16, 2023

Choose a reason for hiding this comment

jayfoad Oct 11, 2023

Choose a reason for hiding this comment

jayfoad commented Oct 11, 2023

nikic commented Oct 11, 2023

bjope commented Oct 12, 2023