Arrrg, mashed send too soon... Forgot I was in an email rather than phabricator.
On Wed, Mar 26, 2014 at 2:58 PM, Chandler Carruth <[email protected]>wrote: > Generally, this seems fine. Some comments about the patch itself: > > + // Check that we only have six bits. > + assert(unsigned(Type) < 1u << NumBitsPerType && > + "Hash is invalid: too many types"); > > I would love parentheses around the << operator as otherwise my brain > struggles with the precedence rules. > > Also, maybe a complementary static_assert on the enum itself? > > + static_assert(!FunctionLikeDecl, "FunctionLikeDecl should have value > 0"); > > I would find comparing with 0 more clear than using ! on an enum... Not a > big deal though > + // Pass through MD5 if enough work has built up. + if (Count && !(Count % NumTypesPerWord)) { Ditto, I would compare the remainder to zero... +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? 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). -Chandler
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
