[PATCH] D40891: Fix accidentally quadratic time in BlockFrequencyInfoImpl::propagateMassToSuccessors

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith requested changes to this revision.
dexonsmith added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/Analysis/BlockFrequencyInfoImpl.h:1318
+    for (auto SuccI = GraphTraits<const BlockT *>::child_begin(BB),
+         SuccE = GraphTraits<const BlockT *>::child_end(BB);
+         SuccI != SuccE;
----------------
If you run `clang-format-diff` on this I think the loop logic will be clearer.  For reference, see the Script for patch reformatting section of:
https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

(It will indent `SuccE`, clarifying that it's a continuation of the statement on the previous line.  I suspect it'll line it up with `SuccI`.)


================
Comment at: include/llvm/Analysis/BlockFrequencyInfoImpl.h:1322
+      if (!addToDist(Dist, OuterLoop, Node, getNode(*SuccI),
+        getWeightFromBranchProb(BPI->getEdgeProbability(BB, SuccI))))
+          // Irreducible backedge.
----------------
A comment explaining why we're passing in an iterator here might help to prevent a regression.

But I wonder if you even need this change: can't `getEdgeProbability()` call `BlockT::getIterator` on the reference?  I believe both `BasicBlock` and `MachineBasicBlock` have that function.  In that case, you should leave this function as is and clean up `getEdgeProbability()`.


Repository:
  rL LLVM

https://reviews.llvm.org/D40891