<div dir="ltr"><div class="gmail_extra">Arrrg, mashed send too soon... Forgot I was in an email rather than phabricator.<br><br><div class="gmail_quote">On Wed, Mar 26, 2014 at 2:58 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Generally, this seems fine. Some comments about the patch itself:<div>
<br></div><div><div>+  // Check that we only have six bits.</div><div>+  assert(unsigned(Type) < 1u << NumBitsPerType &&</div>
<div>+         "Hash is invalid: too many types");</div></div><div><br></div><div>I would love parentheses around the << operator as otherwise my brain struggles with the precedence rules.</div><div><br></div>

<div>Also, maybe a complementary static_assert on the enum itself?</div><div><br></div><div>+  static_assert(!FunctionLikeDecl, "FunctionLikeDecl should have value 0");<br></div><div><br></div><div>I would find comparing with 0 more clear than using ! on an enum... Not a big deal though</div>
</div></blockquote><div><br></div><div><div>+  // Pass through MD5 if enough work has built up.</div><div>+  if (Count && !(Count % NumTypesPerWord)) {</div></div><div><br></div><div>Ditto, I would compare the remainder to zero...</div>
<div><br></div><div><div>+uint64_t PGOHash::finalize() {</div><div>+  // Use Working as the hash directly if we never used MD5.</div><div>+  if (Count <= NumTypesPerWord)</div><div>+    return Working;</div></div><div>
<br></div><div>Doesn't this need to byteswap to LE in the event of a BE host?</div><div><br></div><div><br></div><div>And then one comment that has nothing to do with the patch, but is just a general comment going forward:</div>
<div><br></div><div><div>+/// \brief Stable hasher for PGO region counters.</div><div>+///</div><div>+/// PGOHash produces a stable hash of a given function's control flow.</div><div>+///</div><div>+/// Changing the output of this hash will invalidate all previously generated</div>
<div>+/// profiles -- i.e., don't do it.</div></div><div><br></div><div>Is there some version somewhere that we can bump once a <insert epoch> and change this? I mention this because at some point, we'll want >6 bits, or we'll want to switch from MD5 to MDN+1 or whatever, and we should have *some* recourse for updating it on the scale of O(years).</div>
<div><br></div><div>-Chandler</div></div></div></div>