Skip to content

Conversation

artagnon
Copy link
Contributor

@artagnon artagnon commented Sep 5, 2024

Since no passes compute DependenceAnalysis via the PassManager, there is no value in preserving it here. Hence, strip the unnecessary dependency on DependenceAnalysis.

@llvmbot
Copy link
Member

llvmbot commented Sep 5, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Ramkumar Ramachandra (artagnon)

Changes

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

1 Files Affected:

  • (modified) llvm/lib/Transforms/Utils/LoopSimplify.cpp (-11)
diff --git a/llvm/lib/Transforms/Utils/LoopSimplify.cpp b/llvm/lib/Transforms/Utils/LoopSimplify.cpp
index 5e69923fd989dd..7b8be801a652c0 100644
--- a/llvm/lib/Transforms/Utils/LoopSimplify.cpp
+++ b/llvm/lib/Transforms/Utils/LoopSimplify.cpp
@@ -43,18 +43,13 @@
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
-#include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/Analysis/AssumptionCache.h"
-#include "llvm/Analysis/BasicAliasAnalysis.h"
 #include "llvm/Analysis/BranchProbabilityInfo.h"
-#include "llvm/Analysis/DependenceAnalysis.h"
-#include "llvm/Analysis/GlobalsModRef.h"
 #include "llvm/Analysis/InstructionSimplify.h"
 #include "llvm/Analysis/LoopInfo.h"
 #include "llvm/Analysis/MemorySSA.h"
 #include "llvm/Analysis/MemorySSAUpdater.h"
 #include "llvm/Analysis/ScalarEvolution.h"
-#include "llvm/Analysis/ScalarEvolutionAliasAnalysis.h"
 #include "llvm/IR/CFG.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Dominators.h"
@@ -756,13 +751,8 @@ namespace {
       AU.addRequired<LoopInfoWrapperPass>();
       AU.addPreserved<LoopInfoWrapperPass>();
 
-      AU.addPreserved<BasicAAWrapperPass>();
-      AU.addPreserved<AAResultsWrapperPass>();
-      AU.addPreserved<GlobalsAAWrapperPass>();
       AU.addPreserved<ScalarEvolutionWrapperPass>();
-      AU.addPreserved<SCEVAAWrapperPass>();
       AU.addPreservedID(LCSSAID);
-      AU.addPreserved<DependenceAnalysisWrapperPass>();
       AU.addPreservedID(BreakCriticalEdgesID);  // No critical edges added.
       AU.addPreserved<BranchProbabilityInfoWrapperPass>();
       AU.addPreserved<MemorySSAWrapperPass>();
@@ -849,7 +839,6 @@ PreservedAnalyses LoopSimplifyPass::run(Function &F,
   PA.preserve<DominatorTreeAnalysis>();
   PA.preserve<LoopAnalysis>();
   PA.preserve<ScalarEvolutionAnalysis>();
-  PA.preserve<DependenceAnalysis>();
   if (MSSAAnalysis)
     PA.preserve<MemorySSAAnalysis>();
   // BPI maps conditional terminators to probabilities, LoopSimplify can insert

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The legacy pass is still used by the backend, and at least some of these preservations are important.

@artagnon artagnon removed the request for review from kazutakahirata September 5, 2024 12:21
@artagnon
Copy link
Contributor Author

artagnon commented Sep 5, 2024

The legacy pass is still used by the backend, and at least some of these preservations are important.

The CI failures should be fixed now, but how can we be confident that the patch is correct? What is this mysterious preservation?

@fhahn
Copy link
Contributor

fhahn commented Sep 12, 2024

The legacy pass is still used by the backend, and at least some of these preservations are important.

The CI failures should be fixed now, but how can we be confident that the patch is correct? What is this mysterious preservation?

Probably mostly in extra compile-time, if later passes now have to recompute otherwise preserved info. It might also impact codegen, e.g. GlobalsAAWrapperPass is stateful IIRC, so not preserving it might throw away previous analysis results without recomputing them. The later could be checked by building a large set of programs and checking if there are binary changes

@artagnon artagnon force-pushed the loopsimplify-strip-nfc branch from d88adb9 to 63dc78e Compare September 16, 2024 09:01
@artagnon artagnon changed the title LoopSimplify: strip unused pass dependencies (NFC) LoopSimplify: strip dependency on DA (NFC) Sep 16, 2024
@artagnon
Copy link
Contributor Author

The legacy pass is still used by the backend, and at least some of these preservations are important.

The CI failures should be fixed now, but how can we be confident that the patch is correct? What is this mysterious preservation?

Probably mostly in extra compile-time, if later passes now have to recompute otherwise preserved info. It might also impact codegen, e.g. GlobalsAAWrapperPass is stateful IIRC, so not preserving it might throw away previous analysis results without recomputing them. The later could be checked by building a large set of programs and checking if there are binary changes

Thanks for the explanation! I've now changed this patch to just strip DA (which was my original goal).

@artagnon
Copy link
Contributor Author

Gentle ping. Is this patch okay now?

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. It looks like passes that use DependenceAnalysis don't compute it via the pass manager anyway. It's possible that what LoopSimplify is doing could be useful in the future if we have passes using DA via the PM, but for now this looks fine to me.

@artagnon artagnon merged commit 9f6f6af into llvm:main Oct 1, 2024
8 checks passed
@artagnon artagnon deleted the loopsimplify-strip-nfc branch October 1, 2024 15:25
Sterling-Augustine pushed a commit to Sterling-Augustine/llvm-project that referenced this pull request Oct 3, 2024
Since no passes compute DependenceAnalysis via the PassManager, there is
no value in preserving it here. Hence, strip the unnecessary dependency
on DependenceAnalysis.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants