[llvm] r239843 - Fix PR 23525 - Separate header mass propagation in irregular loops.

[Duncan P. N. Exon Smith]

> On 2015 Jun 17, at 07:11, Diego Novillo <dnovillo at google.com> wrote:
> 
> On Tue, Jun 16, 2015 at 5:00 PM, Duncan P. N. Exon Smith
> <dexonsmith at apple.com> wrote:
>> 
>>> On 2015-Jun-16, at 12:10, Diego Novillo <dnovillo at google.com> wrote:
>>> @@ -223,6 +226,14 @@ public:
>>>    BlockNode getHeader() const { return Nodes[0]; }
>>>    bool isIrreducible() const { return NumHeaders > 1; }
>>> 
>>> +    HeaderMassList::difference_type headerIndexFor(const BlockNode &B) {
>> 
>> Should be a verb, something like `getHeaderIndex()`.
> 
> Done.
> 
>> Can you describe the new behaviour somewhere?  I think it belongs somewhere
>> inside:
>> 
>>    ///  2. Calculate mass and scale in loops (\a computeMassInLoops()).
> 
> 
> Done.
> 
>>> Added: llvm/trunk/test/Analysis/BlockFrequencyInfo/PR23525.ll
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/BlockFrequencyInfo/PR23525.ll?rev=239843&view=auto
>>> ==============================================================================
>>> --- llvm/trunk/test/Analysis/BlockFrequencyInfo/PR23525.ll (added)
>>> +++ llvm/trunk/test/Analysis/BlockFrequencyInfo/PR23525.ll Tue Jun 16 14:10:58 2015
>> 
>> Please rename this testcase to something that describes what you're
>> testing.  Maybe irreducible-backedge-mass.ll?
>> 
>> The PR can go in a comment at the top of the file.
> 
> Sure, I named it based on what I've seen in other test directories. I
> found a couple hundred tests using this naming structure. Is there an
> established convention for this?

Yup, testcase naming in LLVM is all over the place AFAICT.  I don't
think anything is codified.

IMO, descriptive names are far more useful.  I've had the misfortune
of doing a lot of testcase maintenance (for debug info related stuff),
and the PR-based names really bother me.  The faster I can get cues
about what's really being tested, the easier it is to update these
things (and distinguish between having inserted a bug vs bitrotted the
testcase).

> Actually, I don't even think we need this new test case. I had to
> adjust irreducible.ll because the compiler now computes a better
> answer for something irreducible.ll was explicitly stating as wrong
> before.  Should I just leave the test in irreducible.ll? No point
> testing the same thing twice.

Makes sense!

> 
>> I think you should choose more descriptive label names.
> 
> This testcase was generated from a .c file. It has the labels that the
> FE generates.

I understand that clang generated the labels -- my point was that you
should choose more descriptive ones to make the BFI analysis easier
to follow and verify.  Moot point now anyway!

> 
>>> -; CHECK-NEXT: left: float = 3.0,
>>> +; CHECK-NEXT: left: float = 0.14{{[0-9]*}},
>> 
>> I think you can leave this as:
>> 
>>    CHECK-NEXT: left: float = 0.14
> 
> Done.
> 
>>> -; CHECK-NEXT: right: float = 3.0,
>>> +; CHECK-NEXT: right: float = 0.42{{[0-9]*}},
>> 
>> same here
> 
> Done.
> 
>>> top:
>>> -; CHECK-NEXT: top: float = 3.0,
>>> +; CHECK-NEXT: top: float = 8.43{{[0-9]*}},
>> 
>> same here
> 
> Done.
> 
> Patch attached.  OK for trunk?

LGTM.  Noticed one more thing:

> diff --git a/lib/Analysis/BlockFrequencyInfoImpl.cpp b/lib/Analysis/BlockFrequencyInfoImpl.cpp
> index 88fbf7c..c4a7552 100644
> --- a/lib/Analysis/BlockFrequencyInfoImpl.cpp
> +++ b/lib/Analysis/BlockFrequencyInfoImpl.cpp
> @@ -414,7 +414,7 @@ void BlockFrequencyInfoImplBase::distributeMass(const BlockNode &Source,
>  
>      // Check for a backedge.
>      if (W.Type == Weight::Backedge) {
> -      auto ix = OuterLoop->headerIndexFor(W.TargetNode);
> +      auto ix = OuterLoop->getHeaderIndex(W.TargetNode);

Missed this in the original commit.  LLVM coding style uses initial
capitals for all variables (all-caps for initialisms).  Not sure if you
really need a variable here -- you could just roll it into the next line
-- but if you do, something like `I` or `Index` or `HI` or `HeaderIndex`
is better.

>        OuterLoop->BackedgeMass[ix] += Taken;
>        DEBUG(debugAssign(*this, D, W.TargetNode, Taken, "back"));
>        continue;