[PATCH] InstrProf: Calculate a better function hash

[Chandler Carruth]

On Wed, Mar 26, 2014 at 3:53 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

> 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.
>

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...


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

Yea, this part makes sense.


>
> Does this deserve a comment somewhere?
>

Based on my comment above, yea, because I still don't get it. =D


>
> > 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.


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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140326/9cc3c578/attachment.html>