[PATCH] blockfreq: Rewrite block frequency analysis

[Andrew Trick]

On Apr 11, 2014, at 4:47 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:

> 
> On Apr 3, 2014, at 14:11, Andrew Trick <atrick at apple.com> wrote:
> 
>> 
>> On Mar 26, 2014, at 5:43 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> 
>>> 
>>> On Mar 25, 2014, at 7:23 PM, Chandler Carruth <chandlerc at google.com> wrote:
>>> 
>>>> 
>>>> Ok. I think having the terminology clarified will also help me understand the code.
>>>> 
>>>> 
>>>> The only thing that I think really needs to be sorted out (other than the stuff you've clarified you're planning to do already) prior to commit is the positive float issue. I'm just a bit hesitant to put that into Support only to rip it back out. ;] But maybe that doesn't matter too much.
>>> 
>>> FYI, I committed the preparatory patches recently: r204740 and r204741.
>>> 
>>> Here’s a new plan for the remainder that minimizes churn in Support:
>>> 
>>> 1. Move BlockMass and PositiveFloat into private headers.  Strip their
>>>  tests from the patch, but hang onto them out of tree.
>>> 
>>> 2. Commit everything as a new class that’s unused.
>>> 
>>> 3. Move non-trivial things I need from BlockMass and PositiveFloat into
>>>  Support (sharing implementation details with APFloat, where possible). 
>>>  I’ll commit tests for the functionality I need.
>>> 
>>> 4. Switch from soft-float to power-of-two-except-3 loop-scale.  (Delete
>>>  PositiveFloat at this point.)
>>> 
>>> 5. Switch to LoopInfo.
>>> 
>>> 6. Respond to the rest of the review comments incrementally.
>>> 
>>> 7. Turn it on.
>>> 
>>> 8. Kill the old one.
>>> 
>>> Sound good?
>> 
>> It would be great to get a working design committed and begin making the improvements we discussed. However, I don’t think you need to keep the existing implementation alive as you replace pieces. There’s isn’t any need here to preserve old broken behavior. Ideally the first commit will be usable and easy to review even if it’s not efficient or missing features.
>> 
>> 1. Move to using existing LoopInfo to rule out that as a source of problems.
>> 
>> 2. Move BlockMass and PositiveFloat out of support (make them temporary helpers)
>> 
>> 3. Commit and enable.
>> 
>> 4. Factor Float/Mass utilities into Support
>> 
>> 5. Switch to pow2 loop scale
>> 
>> 6. Reimplement Positive float as a very simple BlockFrequency-specific wrapper that implements loop scale computation and frequency scaling in terms of integer operations (you seem to have come up with a way to do this that won’t lead to madness).
>> 
>> One more thing. Chandler requested definitions of the terminology, which would be good to do before your first commit.
>> 
>> -Andy
> 
> Andy: thanks, this plan is better.
> 
> I just pushed a bunch of prep commits, including the terminology, in
> r206082, r206083, r206084, r206085, and r206086.
> 
> I'm ready to commit the attached patch, which finishes off steps 1-3.
> I ran SPEC2000 on ARM64 and saw a slight improvement overall.
> 
> Chandler: is it okay with you to move this review to post-commit?
> It'll be easier to work on it incrementally in trunk, and I think it's
> an improvement over what's there.  Or do you want another pass at this
> first via phabricator?
> 
> <block-frequency-20140411.patch>

Thanks. I’m fine if you continue working on this in-tree since you're tracking performance.

At this point, we mainly want to
- simplify the code by moving helper code into support or eliminating the need for it (especially PositiveFloat)
- add the block bias computation and API
- maybe do a little better on irreducible CFGs

-Andy