[PATCH] D136827: [JT][CT] Preserve exisiting BPI/BFI during JumpThreading

[Evgeniy via Phabricator via llvm-commits]

ebrevnov marked 3 inline comments as done.
ebrevnov added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Scalar/JumpThreading.h:78
 class JumpThreadingPass : public PassInfoMixin<JumpThreadingPass> {
-  TargetLibraryInfo *TLI;
-  TargetTransformInfo *TTI;
-  LazyValueInfo *LVI;
-  AAResults *AA;
-  DomTreeUpdater *DTU;
-  std::unique_ptr<BlockFrequencyInfo> BFI;
-  std::unique_ptr<BranchProbabilityInfo> BPI;
-  bool HasProfileData = false;
+  Function *F = nullptr;
+  FunctionAnalysisManager *FAM = nullptr;
----------------
mkazantsev wrote:
> Can be &Function (less changes below)
> Can be &Function (less changes below)




================
Comment at: llvm/include/llvm/Transforms/Scalar/JumpThreading.h:78
 class JumpThreadingPass : public PassInfoMixin<JumpThreadingPass> {
-  TargetLibraryInfo *TLI;
-  TargetTransformInfo *TTI;
-  LazyValueInfo *LVI;
-  AAResults *AA;
-  DomTreeUpdater *DTU;
-  std::unique_ptr<BlockFrequencyInfo> BFI;
-  std::unique_ptr<BranchProbabilityInfo> BPI;
-  bool HasProfileData = false;
+  Function *F = nullptr;
+  FunctionAnalysisManager *FAM = nullptr;
----------------
ebrevnov wrote:
> mkazantsev wrote:
> > Can be &Function (less changes below)
> > Can be &Function (less changes below)
> 
> 
No, it can't. It's not initialized in the constructor.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:357
+#ifndef NDEBUG
+  DTU.flush();
+#if defined(EXPENSIVE_CHECKS)
----------------
mkazantsev wrote:
> Why this is under NDEBUG?
Because we need to flush DT at this point only if verification follows. Otherwise it's up to DTU to decide when to flush.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:374
+  // TODO: Would be nice to verify BPI/BFI consistency similar to DominatorTree.
+//  PA.preserve<BranchProbabilityAnalysis>();
+//  PA.preserve<BlockFrequencyAnalysis>();
----------------
mkazantsev wrote:
> Bad formatting
Not sure what LLVM Coding Standard says about that situation but I intentionally formatted this way to keep visual separation between real comments and commented out code.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:1133
     ConstantFoldTerminator(BB, true, nullptr, DTU);
-    if (HasProfileData)
+    if (auto BPI = getBPI())
       BPI->eraseBlock(BB);
----------------
mkazantsev wrote:
> Is it guaranteed that BPI is created by this moment? How?
> 
> Should be getOrCreateBPI?
No it's not guaranteed. JT doesn't use BPI/BFI for its needs. It only creates it to update BPI/BFI and profile metadata if exists. Thus if BPI exists it will be updated, otherwise no need to do anything.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136827