[PATCH] InstrProf: Calculate a better function hash

[Duncan P. N. Exon Smith]

ping

On Mar 26, 2014, at 16:41, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:

> 
> On Mar 26, 2014, at 4:02 PM, Chandler Carruth <chandlerc at google.com> wrote:
> 
>> 
>> 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.
> 
> New patch attached; I think it addresses all of your comments.
> 
> <pgo-hash-3.patch>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pgo-hash-3.patch
Type: application/octet-stream
Size: 33153 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140402/993a05b4/attachment.obj>