[llvm] r320208 - Revert part of "Cleanup some GraphTraits iteration code"

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

> On Dec 8, 2017, at 15:38, Jonathan Roelofs <jonathan at codesourcery.com> wrote:
> 
> 
> 
> On 12/8/17 3:42 PM, Duncan P. N. Exon Smith via llvm-commits wrote:
>> Author: dexonsmith
>> Date: Fri Dec  8 14:42:43 2017
>> New Revision: 320208
>> URL: http://llvm.org/viewvc/llvm-project?rev=320208&view=rev
>> Log:
>> Revert part of "Cleanup some GraphTraits iteration code"
>> This reverts part of r300656, which caused a regression in
>> propagateMassToSuccessors by counting edges n^2 times, where n is the
>> number of edges from the source basic block to the same successor basic
>> block. The result was both incorrect and very slow to compute for large
>> values of n (e.g. switches with multiple cases that go to the same basic
>> block).
>> Patch by Andrew Scheidecker!
>> Added:
>>     llvm/trunk/test/Analysis/BlockFrequencyInfo/redundant_edges.ll
>> Modified:
>>     llvm/trunk/include/llvm/Analysis/BlockFrequencyInfoImpl.h
>> Modified: llvm/trunk/include/llvm/Analysis/BlockFrequencyInfoImpl.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/BlockFrequencyInfoImpl.h?rev=320208&r1=320207&r2=320208&view=diff
>> ==============================================================================
>> --- llvm/trunk/include/llvm/Analysis/BlockFrequencyInfoImpl.h (original)
>> +++ llvm/trunk/include/llvm/Analysis/BlockFrequencyInfoImpl.h Fri Dec  8 14:42:43 2017
>> @@ -1314,9 +1314,12 @@ BlockFrequencyInfoImpl<BT>::propagateMas
>>        return false;
>>    } else {
>>      const BlockT *BB = getBlock(Node);
>> -    for (const auto Succ : children<const BlockT *>(BB))
>> -      if (!addToDist(Dist, OuterLoop, Node, getNode(Succ),
>> -                     getWeightFromBranchProb(BPI->getEdgeProbability(BB, Succ))))
>> +    for (auto SI = GraphTraits<const BlockT *>::child_begin(BB),
>> +              SE = GraphTraits<const BlockT *>::child_end(BB);
>> +         SI != SE; ++SI)
>> +      if (!addToDist(
>> +              Dist, OuterLoop, Node, getNode(*SI),
>> +              getWeightFromBranchProb(BPI->getEdgeProbability(BB, SI))))
> 
> 
> Where was the bug here, does children() recompute more or something?

It would have helped if I'd linked to the review in the commit message:
https://reviews.llvm.org/D40891 <https://reviews.llvm.org/D40891>

- `getEdgeProbability(BasicBlock&, edge_iterator)` returns the probability of a single edge.
- `getEdgeProbability(BasicBlock&, BasicBlock&)` returns the sum of the edges between blocks.


> 
> 
> Jon
> 
>>          // Irreducible backedge.
>>          return false;
>>    }
>> Added: llvm/trunk/test/Analysis/BlockFrequencyInfo/redundant_edges.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/BlockFrequencyInfo/redundant_edges.ll?rev=320208&view=auto
>> ==============================================================================
>> --- llvm/trunk/test/Analysis/BlockFrequencyInfo/redundant_edges.ll (added)
>> +++ llvm/trunk/test/Analysis/BlockFrequencyInfo/redundant_edges.ll Fri Dec  8 14:42:43 2017
>> @@ -0,0 +1,22 @@
>> +; RUN: opt < %s -analyze -block-freq | FileCheck %s
>> +; RUN: opt < %s -analyze -lazy-block-freq | FileCheck %s
>> +; RUN: opt < %s -passes='print<block-freq>' -disable-output 2>&1 | FileCheck %s
>> +
>> +define void @test1() {
>> +; CHECK-LABEL: Printing analysis {{.*}} for function 'test1':
>> +; CHECK-NEXT: block-frequency-info: test1
>> +; CHECK-NEXT: entry: float = 1.0, int = [[ENTRY:[0-9]+]]
>> +entry:
>> +  br label %loop
>> +
>> +; CHECK-NEXT: loop: float = 32.0
>> +loop:
>> +  switch i32 undef, label %loop [
>> +    i32 0, label %return
>> +    i32 1, label %return
>> +  ]
>> +
>> +; CHECK-NEXT: return: float = 1.0
>> +return:
>> +  ret void
>> +}
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> 
> -- 
> Jon Roelofs
> jonathan at codesourcery.com <mailto:jonathan at codesourcery.com>
> CodeSourcery / Mentor Embedded / Siemens

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171208/661e0962/attachment-0001.html>