ping On 2014 Apr 2, at 14:35, Duncan P. N. Exon Smith <[email protected]> wrote:
> ping > > On Mar 26, 2014, at 16:41, Duncan P. N. Exon Smith <[email protected]> > wrote: > >> >> On Mar 26, 2014, at 4:02 PM, Chandler Carruth <[email protected]> wrote: >> >>> >>> On Wed, Mar 26, 2014 at 3:53 PM, Duncan P. N. Exon Smith >>> <[email protected]> wrote: >>> On Mar 26, 2014, at 3:12 PM, Chandler Carruth <[email protected]> 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 >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > <pgo-hash-3.patch>
pgo-hash-3.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
