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

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

dexonsmith added a comment.

In https://reviews.llvm.org/D40891#947361, @AndrewScheidecker wrote:

> I ran the patch through clang-format. But looking at the code again, I see that calling the `getEdgeProbability` overload with a successor index or iterator instead of the destination basic block actually does something different from the original code: passing a successor index/iterator gets the probability for a single edge, while passing a basic block sums the probability for all edges between the two blocks.
>
> However, and I have to admit that I'm in over my head here, what the original code was doing doesn't seem correct. For each edge from A to B, it was summing the probability of all edges from A to B, then adding the sum to the mass for B. If there are N edges from A to B, and they have probabilities that sum to M, then this will distribute N*M mass to B instead of M. In my contrived case, N=196928.
>
> I still think this change is good, but I didn't mean for it to change the results. What are your thoughts?


You're right, the current logic is wrong.  Looking at the history, it looks like it was broken by r300656, which was reviewed in https://reviews.llvm.org/D32118.  You're effectively doing a partial-revert of that commit -- please be clear about that in the commit message.

Also, please add a test for this.  See the current tests in `test/Analysis/BlockFrequencyInfo/` for examples.

As a side note, it's somewhat subtle for the edge-iterator and block-reference versions of `getEdgeProbability` to give different answers.  I think the latter should change names, perhaps to `getEdgeProbabilitySum`.  But that should be in a follow-up patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D40891