[PATCH] InstrProf: Calculate a better function hash

[Duncan P. N. Exon Smith]

On Mar 26, 2014, at 3:12 PM, Chandler Carruth <chandlerc at google.com> wrote:

> +uint64_t PGOHash::finalize() {
> +  // Use Working as the hash directly if we never used MD5.
> +  if (Count <= NumTypesPerWord)
> +    return Working;
> 
> Doesn't this need to byteswap to LE in the event of a BE host?

I don’t think so.

All the math on Working is 64-bit integer math.  In IRGen with
-fprofile-instr-generate, it's emitted as an i64.  The various writers
and readers treat it as a 64-bit value (byte swapping as necessary)
until -fprofile-instr-use sees it on the other side.

We do need to byte swap the inputs and outputs to MD5, since its
interface uses uint8_t.

Does this deserve a comment somewhere?

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

Yeah, this is essential.  The idea is to tie it to the version number
in the indexed binary format, which unfortunately doesn’t exist yet.