[PATCH] blockfreq: Rewrite block frequency analysis

[Duncan P. N. Exon Smith]

On Mar 19, 2014, at 12:29 AM, Andrew Trick <atrick at apple.com> wrote:

> 
> On Mar 7, 2014, at 3:44 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
>> This patch series rewrites the shared implementation of BlockFrequencyInfo
>> and MachineBlockFrequencyInfo entirely.
>> 
>> - Patches 0001 and 0002 are simple cleanups.
>> 
>> - Patch 0003 is a class rename (and file move).  Let me know if I should
>>   just merge it with 0006.
>> 
>> - Patches 0004 and 0005 introduce some supporting classes.
>> 
>> - Patch 0006 rewrites BlockFrequencyInfoImpl entirely.
>> 
>> - Patch 0007 is *not* included (it’s headed separately to llvmdev).
> 
> These patches all look pretty great to me.

Thanks everyone for the reviews so far.  There seems to be some consensus that
the direction of PositiveFloat is a good one (as long as it shares effort with
APFloat to minimize maintenance burden), and I’ve had a couple of LGTMs on the
overall algorithm.

What’s the path forward?  I propose the following:

 0. clang-format :).

 1. Commit patches 0001/0002.

 2. Put the math from PositiveFloat into reusable helper class (or namespace?),
    and factor out the common components from APFloat.  I'll work on this
    incrementally in the tree, and port my existing testcases from
    PositiveFloat as necessary.

 3. Add the math from BlockMass that needs testing to the same helper class
    (namespace?).  This will allow BlockMass to be a local implementation
    detail of BlockFrequencyInfoImpl, sidestepping concerns about lib/Support
    bloat.

 4. Commit PositiveFloat.cpp, which will at this point be mostly API boilerplate
    (operators, etc.).

 5. Commit BlockFrequencyInfoImpl in the new location, leaving the old
    BlockFrequencyImpl in place and in use.  Incrementally apply review feedback
    starting with what I’ve already received in this thread and the RFC.

 6. “Turn on” the new pass implementation, and then delete the old one.

 7. Commit the bias metric (current patch 0007), so that people can experiment
    with using that instead of block frequency.

Does this sound good to everyone?

Chandler, do you still want something up on Phab at some point?  If so, does it
makes sense for me to post the current, massive, combined patch of
0003+0004+0005+0006(+0007?) now, so you can look at the algorithm as a whole?