[PATCH] D84838: [BPI][NFC] Unify handling of normal and SCC based loops

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added inline comments.


================
Comment at: llvm/include/llvm/Analysis/BranchProbabilityInfo.h:258
+  // Pair of LoopBlocks representing an edge from first to second block.
+  using LoopEdge = std::pair<const LoopBlock &, const LoopBlock &>;
+
----------------
davidxl wrote:
> Perhaps make it a class and make related methods below members?
Similarly to LoopBlock, I'd like to keep it simple and don't populate it with a reference to LoopInfo/SccInfo.


================
Comment at: llvm/include/llvm/Analysis/BranchProbabilityInfo.h:292
+  /// other cases.
+  bool isDifferentLoopsEdge(const LoopEdge &Edge) const;
+  /// Returns true if destination block belongs to some loop and source block is
----------------
davidxl wrote:
> perhaps "isLoopCrossingEdge' or isLoopEnteringExitingEdge'?
I don't like current name either. isLoopEnteringExitingEdge sounds much better for me. Naturally it can be expressed via isLoopEntering/ExitingEdge. Thanks for the suggestion.


================
Comment at: llvm/include/llvm/Analysis/BranchProbabilityInfo.h:305
+  // Fills in \p Enters vector with all "enter" blocks to a loop \LB belongs to.
+  void getLoopEnterBlocks(const LoopBlock &LB,
+                          SmallVectorImpl<BasicBlock *> &Enters) const;
----------------
davidxl wrote:
> make it a member of the LoopBlock. Also for consistency: getLoopEnteringBlocks.
Moving it to LoopBlock will require keeping a reference to SccInfo from LoopBlock which I'd like to avoid and keep LoopBlock as light wight as possible (similarly, Loop doesn't keep a reference to LoopInfo).

I'm trying to keep same naming convention as we use for exits here. Loop exiting block is a block which belongs to a loop and has an edge to exit block.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84838