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

[Evgeniy via Phabricator via llvm-commits]

ebrevnov marked an inline comment as done.
ebrevnov added inline comments.


================
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:
> ebrevnov wrote:
> > 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.
> Maybe just write "we want to preserve BranchProbabilityAnalysis and BlockFrequencyAnalysis here..." and not leave the commented code? Any static analyzer will complain about it.
Sure, can do that.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:3128
+    // BlockFrequencyAnalysis depends on DT. Make sure DT is consistent.
+    DTU->flush();
+    BFI = &FAM->getResult<BlockFrequencyAnalysis>(*F);
----------------
mkazantsev wrote:
> Same as above, add verification?
> 
> And if you unite them in one method, it's only needed once.
Fixed.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:3106
+
+BlockFrequencyInfo *JumpThreadingPass::getOrCreateBFI() {
+  if (HasProfile && (!BFI || *BFI == nullptr)) {
----------------
mkazantsev wrote:
> ebrevnov wrote:
> > mkazantsev wrote:
> > > They are only called together; do we really need two separate functions?
> > There are number of place where we request BPI only...
> getOrCreateBPI/BFI are only called together.
Indeed, getOrCreateBPI/BFI are called together at the moment but it can change in the future. I feel in favor of leaving it as is. I don't think this is super critical. Let's leave as is if you don't mind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136827