On Sun, Feb 5, 2012 at 4:53 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div class="gmail_quote"><div class="im">On Sun, Feb 5, 2012 at 4:43 PM, Jakob Stoklund Olesen <span dir="ltr"><<a href="mailto:stoklund@2pi.dk" target="_blank">stoklund@2pi.dk</a>></span> wrote:<br></div><div><div>

</div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br>
On Feb 5, 2012, at 4:31 PM, Talin wrote:<br>
<br>
> Patch to refactor the hashing functions for the constant unique map, to avoid code duplication.<br>
<br>
</div>Wouldn't it give nicer code if you kept the intermediate state as a class member?<br>
<br>
Something like:<br>
<br>
IncrementalHash Hash<br>
Hash.add(foo);<br>
Hahs.add(bar);<br>
return Hash.finish();</blockquote><div><br></div></div></div><div>If you're interested in building nice, generalized hashing infrastructure in Support, I think we can and should have a much better interface than this or the one Talin is proposing.</div>


<div><br></div><div>I would look at the proposed standard interfaces for hashing that Jeffrey and I are proposing for the next C++ standard. We could almost certainly implement something very similar to it, although there are a few subtleties we would need to work out surrounding name lookup.</div>


<div><br></div><div>Also, I don't really want such a weak hash function in Support. The standard libraries actually have much higher quality hash functions now.</div><div><br></div><div>Finally, we already have some hashing  routines in the DenseSet implementation. I feel this refactoring is only the tip of the ice berg, and isn't really factored properly for use in the rest of the cases.</div>


<div><br></div><div>However, as Nick hinted, I think the best solution might be to change the code Talin originally touched to use FoldingSetNodeID, which was carefully designed for such situations it seems.... A generalized hash function often isn't designed in that way.</div>


</div>
</blockquote></div><div><br></div>Yeah, I'm aware of the hashing functions in DenseMapInfo and FoldingSetNodeID...I just wasn't sure I wanted to be that ambitious. Given that I'm not an expert on hashing, I was trying to avoid any behavior changes to existing code. I didn't want FoldingSet to start getting collisions because of some mistake I had made.<div>

<br></div><div>There's also Austin Appleby's MurmurHash3 (<a href="http://code.google.com/p/smhasher/">http://code.google.com/p/smhasher/</a>), which is also a much better hashing function. If you look at the source (<a href="http://code.google.com/p/smhasher/source/browse/trunk/MurmurHash3.cpp">http://code.google.com/p/smhasher/source/browse/trunk/MurmurHash3.cpp</a>) you can see where he's a number of tricky optimizations for various compilers. And since the code is public domain, there would be no issue including it in LLVM verbatim.</div>

<div><br></div><div>The one downside is that his implementation doesn't do incremental hashes very well - it operates most efficiently when the data is in a contiguous buffer. That works fine for the FoldingSetNodeID use case, but isn't suitable for the constant uniquing case because you can't get a contiguous array of the operands of a constant. (Internally the constants are stored as an array of Use*, with additional data stored in the low two bits, which we don't want to be included in the hash calculation.)</div>

<div><br></div><div>-- <br>-- Talin<br>
</div>