[PATCH] D40891: Fix accidentally quadratic time in BlockFrequencyInfoImpl::propagateMassToSuccessors
Duncan P. N. Exon Smith via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 6 16:16:42 PST 2017
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
More information about the llvm-commits
mailing list