[PATCH] D78987: [BPI][NFC] Reuse post dominantor tree from analysis manager when available

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

I'm very confused by this patch. This does not look like an NFC.

1. You are introducing a mandatory dependency on the postdom and the testcase changes do not look NFC. Please see inline comments for how to address this. To match your patch description the pass should not be required and you should be getting the cached analysis.

2. This is also adding the creation of a postdom inside BPI when it's not available. This is questionable, for a pass to create a postdom inside and not something I think is acceptable.

If it is in fact needed to add it as required in order to address imprecise results, then motivate that and title/describe the patch appropriately.

Please either revert or address comments, and split up such changes in behavior into separate patches.



================
Comment at: llvm/lib/Analysis/BranchProbabilityInfo.cpp:1067
   AU.addRequired<TargetLibraryInfoWrapperPass>();
+  AU.addRequired<PostDominatorTreeWrapperPass>();
   AU.setPreservesAll();
----------------
Remove


================
Comment at: llvm/lib/Analysis/BranchProbabilityInfo.cpp:1076
+  PostDominatorTree &PDT =
+      getAnalysis<PostDominatorTreeWrapperPass>().getPostDomTree();
+  BPI.calculate(F, LI, &TLI, &PDT);
----------------
`getCachedAnalysis`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78987