<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 26, 2014 at 3:53 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 class="">On Mar 26, 2014, at 3:12 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br>

<br>
> +uint64_t PGOHash::finalize() {<br>
> +  // Use Working as the hash directly if we never used MD5.<br>
> +  if (Count <= NumTypesPerWord)<br>
> +    return Working;<br>
><br>
> Doesn't this need to byteswap to LE in the event of a BE host?<br>
<br>
</div>I don’t think so.<br>
<br>
All the math on Working is 64-bit integer math.  In IRGen with<br>
-fprofile-instr-generate, it's emitted as an i64.  The various writers<br>
and readers treat it as a 64-bit value (byte swapping as necessary)<br>
until -fprofile-instr-use sees it on the other side.<br></blockquote><div><br></div><div>So, I still don't understand how this works: host = LE, target = BE; the IRGen writes an i64 bigendian hash, the profile reader reads an i64 littleendian hash, boom? I'm probably missing a step...</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
We do need to byte swap the inputs and outputs to MD5, since its<br>
interface uses uint8_t.<br></blockquote><div><br></div><div>Yea, this part makes sense.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Does this deserve a comment somewhere?<br></blockquote><div><br></div><div>Based on my comment above, yea, because I still don't get it. =D</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div class=""><br>
> And then one comment that has nothing to do with the patch, but is just a general comment going forward:<br>
><br>
> +/// \brief Stable hasher for PGO region counters.<br>
> +///<br>
> +/// PGOHash produces a stable hash of a given function's control flow.<br>
> +///<br>
> +/// Changing the output of this hash will invalidate all previously generated<br>
> +/// profiles -- i.e., don't do it.<br>
><br>
> 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).<br>

<br>
</div>Yeah, this is essential.  The idea is to tie it to the version number<br>
in the indexed binary format, which unfortunately doesn’t exist yet.</blockquote></div><br>Cool, feel free to document it with a fixme to actually add the version. =D Clearly, we don't have the format available to put it in, but its good to know that it needs to be stable, but there is *some* escape hatch somewhere.</div>
</div>