[PATCH] InstrProf: Calculate a better function hash

[Chandler Carruth]

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>