[PATCH] InstrProf: Calculate a better function hash
Chandler Carruth
chandlerc at google.com
Wed Mar 26 15:12:49 PDT 2014
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 <chandlerc at google.com>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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140326/d1706796/attachment.html>
More information about the cfe-commits
mailing list