<div dir="ltr">Generally, this seems fine. Some comments about the patch itself:<div><br></div><div><div>+  // Check that we only have six bits.</div><div>+  assert(unsigned(Type) < 1u << NumBitsPerType &&</div>
<div>+         "Hash is invalid: too many types");</div></div><div><br></div><div>I would love parentheses around the << operator as otherwise my brain struggles with the precedence rules.</div><div><br></div>
<div>Also, maybe a complementary static_assert on the enum itself?</div><div><br></div><div>+  static_assert(!FunctionLikeDecl, "FunctionLikeDecl should have value 0");<br></div><div><br></div><div>I would find comparing with 0 more clear than using ! on an enum...</div>
<div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Mar 26, 2014 at 2:51 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mar 25, 2014, at 5:39 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>

<br>
> On Mar 25, 2014, at 2:30 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br>
><br>
>><br>
>> On Tue, Mar 25, 2014 at 1:59 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
>>  2. Grab the upper 64 bits of MD5.<br>
>><br>
>> How about this.<br>
>><br>
>> Pack the bits of the enum into a 64-bit integer. When we fill the integer, spin up an MD5 context, and shove the integer through it, rinse, repeat. When done, if you have an MD5, take the upper 64-bits. If not, just take the integer.<br>

>><br>
>> This should incur no cost for the most common case (only a few CFG elements) and no collisions roughly ever, and scale nicely due to using 64-bit chunks.<br>
>><br>
>> s/MD5/any other stable hashing function really/ -- my hope is that after doing this, the performance of the case where all the CFG elements didn't just serialize is relatively unimportant.<br>
>><br>
>> Then, benchmark it, and if MD5 is a problem, revisit it with faster and/or lower overhead algorithms which still have well known and fixed results such as MD4, blake2, etc.<br>
><br>
> Sounds like a great compromise.  I’ll get a new patch posted soon.<br>
<br>
</div></div>Thanks again for the reviews.  Here’s an updated patch.  I chose to leave 6 bits<br>
for the enum to leave space for expansion (so 10 values get packed per 64-bits).<br>
<div class=""><br>
> On Mar 25, 2014, at 2:33 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br>
><br>
>> As a co-worker who also works on hashing just reminded me, this relies on the fact that none of the enum values are 0, otherwise you end up with 'x' and 'yx' for two sequences which trivially collide.<br>

<br>
</div>Note:  there is an enum value that’s zero:  FunctionLikeDecl.  Each time a hash is<br>
calculated, the first (and only the first) counter is assigned to the function<br>
itself (or objective-c block, etc.).  The hash logic effectively skips it.  Only<br>
subsequent (and non-zero) values get combined.<br>
<br>
The enum values that do get combined are all non-zero.<br>
<br>
</blockquote></div><br></div>