[PATCH] blockfreq: Rewrite block frequency analysis

[Chandler Carruth]

On Fri, 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.
>

These LGTM, however I would generally prefer the formatting of clang-format
(after PR17999 is fixed perhaps) to your current formatting. Especially, I
really dislike the breaking after the class name in a the out-of-line
nested-name-specifier of method definitions. With the formatting issues
addressed, please go ahead and apply them.


>
>   - Patch 0003 is a class rename (and file move).  Let me know if I should
>     just merge it with 0006.
>

I don't really understand what the move accomplishes. Why is "InfoImp"
better that "Impl"? What difference are you trying to make here? I don't
think you should use the suffix of "Imp" in any case as "Impl" is more
widely used in LLVM. I hate the suffix of "Info" as it conveys no meaning
at all. Perhaps there is a fundamentally better name?


>   - Patches 0004 and 0005 introduce some supporting classes.
>

>   - Patch 0006 rewrites BlockFrequencyInfoImpl entirely.
>

I share Bob's concern about PositiveFloat -- I'm worried about introducing
a second quite complex floating point class with subtle and untest
differences from APFloat. This is especially true if one cannot be
implemented in terms of the others.

Either BlockMass should replace BlockFrequency and be used by downstream
consumers of the analysis pass, or it should be an in internal class used
by the implementation of the BFI analysis and not in a public Support
header. It's not clear to me which direction you're aiming at?

I'm also very concerned about not re-using LoopInfo. I saw your
description, but the fact that you weren't able to use LoopInfo for this
seems like a critical failure in its design, and not something we should
paper over. I haven't yet gotten through enough of the patch to fully
understand the problem though. Essentially, I would like to review all of
the last three more carefully, but honestly they are *huge* patches. Would
you be OK posting them in Phabricator which makes reviewing particularly
sizable new bodies of code somewhat easier?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140317/7ec17dcc/attachment.html>