<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Mar 7, 2014 at 3:44 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div id=":4bh" class="a3s" style="overflow:hidden">This patch series rewrites the shared implementation of BlockFrequencyInfo<br>

and MachineBlockFrequencyInfo entirely.<br>
<br>
  - Patches 0001 and 0002 are simple cleanups.<br></div></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div id=":4bh" class="a3s" style="overflow:hidden">
<br>
  - Patch 0003 is a class rename (and file move).  Let me know if I should<br>
    just merge it with 0006.<br></div></blockquote><div><br></div><div>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?</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div id=":4bh" class="a3s" style="overflow:hidden">
<br>
  - Patches 0004 and 0005 introduce some supporting classes.</div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div id=":4bh" class="a3s" style="overflow:hidden">

<br>
  - Patch 0006 rewrites BlockFrequencyInfoImpl entirely.<br></div></blockquote></div><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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?</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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?</div>
</div>