<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Mar 19, 2014, at 1:38 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br>On Mar 19, 2014, at 12:29 AM, Andrew Trick <<a href="mailto:atrick@apple.com">atrick@apple.com</a>> wrote:<br><br><blockquote type="cite"><br>On Mar 7, 2014, at 3:44 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br><br><blockquote type="cite">This patch series rewrites the shared implementation of BlockFrequencyInfo<br>and MachineBlockFrequencyInfo entirely.<br><br>- Patches 0001 and 0002 are simple cleanups.<br><br>- Patch 0003 is a class rename (and file move). Let me know if I should<br> just merge it with 0006.<br><br>- Patches 0004 and 0005 introduce some supporting classes.<br><br>- Patch 0006 rewrites BlockFrequencyInfoImpl entirely.<br><br>- Patch 0007 is *not* included (it’s headed separately to llvmdev).<br></blockquote><br>These patches all look pretty great to me.<br></blockquote><br>Thanks everyone for the reviews so far. There seems to be some consensus that<br>the direction of PositiveFloat is a good one (as long as it shares effort with<br>APFloat to minimize maintenance burden), and I’ve had a couple of LGTMs on the<br>overall algorithm.<br><br>What’s the path forward? I propose the following:<br><br>0. clang-format :).<br><br>1. Commit patches 0001/0002.<br><br>2. Put the math from PositiveFloat into reusable helper class (or namespace?),<br> and factor out the common components from APFloat. I'll work on this<br> incrementally in the tree, and port my existing testcases from<br> PositiveFloat as necessary.<br><br>3. Add the math from BlockMass that needs testing to the same helper class<br> (namespace?). This will allow BlockMass to be a local implementation<br> detail of BlockFrequencyInfoImpl, sidestepping concerns about lib/Support<br> bloat.<br><br>4. Commit PositiveFloat.cpp, which will at this point be mostly API boilerplate<br> (operators, etc.).<br><br>5. Commit BlockFrequencyInfoImpl in the new location, leaving the old<br> BlockFrequencyImpl in place and in use. Incrementally apply review feedback<br> starting with what I’ve already received in this thread and the RFC.<br></div></blockquote><div><br></div><div>Sounds good to me.</div><div><br></div><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">6. “Turn on” the new pass implementation, and then delete the old one.<br></div></blockquote><div><br></div><div>Yes, after checking compile time and deciding how we should verify the implementation now and after future changes.</div></div><div><br><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">7. Commit the bias metric (current patch 0007), so that people can experiment<br> with using that instead of block frequency.<br></div></blockquote><div><br></div>Yes, after resolving questions on the RFC thread.</div><div><br></div><div>-Andy</div><div><br><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br>Does this sound good to everyone?<br><br>Chandler, do you still want something up on Phab at some point? If so, does it<br>makes sense for me to post the current, massive, combined patch of<br>0003+0004+0005+0006(+0007?) now, so you can look at the algorithm as a whole?</div></blockquote></div><br></body></html>