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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev accepted this revision.
mkazantsev added a comment.
This revision is now accepted and ready to land.

LG in general, good that it won't regress standard builds. Let's give it a try.

Some nits inline, I still think that getOrCreate... could be one method.



================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:357
+#ifndef NDEBUG
+  DTU.flush();
+#if defined(EXPENSIVE_CHECKS)
----------------
ebrevnov wrote:
> 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.
Yeah, but I mean it's the end of the line anyways, and DTU will flush in the end of this function. We could just flush it here.


================
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>();
----------------
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.


================
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);
----------------
Same as above, add verification?

And if you unite them in one method, it's only needed once.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:3100
+    // BranchProbabilityAnalysis depends on DT. Make sure DT is consistent.
+    DTU->flush();
+    BPI = &FAM->getResult<BranchProbabilityAnalysis>(*F);
----------------
ebrevnov wrote:
> mkazantsev wrote:
> > Would be nice to verify DT here in assertion mode.
> I think it makes sense to perform "fast" verification here and "full" at the end of the pass under EXPENSIVE_CHECKS guard. WDYT?
Yes, sounds good.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:3106
+
+BlockFrequencyInfo *JumpThreadingPass::getOrCreateBFI() {
+  if (HasProfile && (!BFI || *BFI == nullptr)) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136827