[PATCH] blockfreq: Rewrite block frequency analysis

[Andrew Trick]

On Mar 19, 2014, at 1:38 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:

> 
> 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.

Sounds good to me.

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

Yes, after checking compile time and deciding how we should verify the implementation now and after future changes.

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

Yes, after resolving questions on the RFC thread.

-Andy

> 
> 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?

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140319/a25b485e/attachment.html>