On Tue, Oct 2, 2012 at 4:22 PM, Michael Ilseman <span dir="ltr"><<a href="mailto:milseman@apple.com" target="_blank" class="cremed">milseman@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div class="h5"><br><div><div>On Oct 1, 2012, at 11:17 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank" class="cremed">chandlerc@google.com</a>> wrote:</div>
<br><blockquote type="cite"><div class="gmail_extra"><div class="gmail_quote">On Mon, Oct 1, 2012 at 10:51 PM, Chris Lattner <span dir="ltr"><<a href="mailto:clattner@apple.com" target="_blank" class="cremed">clattner@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><br>
On Oct 1, 2012, at 1:14 PM, Michael Ilseman <<a href="mailto:milseman@apple.com" target="_blank" class="cremed">milseman@apple.com</a>> wrote:<br>
> I could also swap based on the addresses of the operands, which is how GVN does it. Swapping itself would still change execution to execution, but it will be consistent within a single execution.<br>
<br>
</div>I think that this is the right way to go. The code in EarlyCSE should stay really simple, and I don't like the idea of adding yet-another expression abstraction that has to stay up to date with the IR as we add new things to it.<br>
</blockquote><div><br></div><div>Ok, this is making more sense now. Sorry for my confusion earlier.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">What I'm suggesting is that the hashing code can look like (psuedo code obviously):<br>
<br>
unsigned DenseMapInfo<SimpleValue>::getHashValue(SimpleValue Val) {<br>
Instruction *Inst = Val.Inst;<br>
<br>
// Hash in all of the operands as pointers.<br>
bool Swapped = false;<br>
if (isa<BinaryOperator>(Inst) && Inst->isCommutative() && Inst->getOperand(0) > Inst->getOperand(1)) {<br>
Inst->swapOperands();<br>
Swapped = true;<br>
}<br>
<br>
unsigned Res = 0;<br>
for (unsigned i = 0, e = Inst->getNumOperands(); i != e; ++i)<br>
Res ^= getHash(Inst->getOperand(i)) << (i & 0xF);<br>
<br>
...<br>
<br>
// Mix in the opcode.<br>
Res = (Res << 1) ^ Inst->getOpcode();<br>
<br>
if (Swapped) Inst->swapOperands();<br>
return Res;<br>
}<br>
<br>
Similarly for compares.<br>
<br>
Of course, it would be much better to avoid actually swapping the operands, and I don't think it would make the code any more complex to do that.<br></blockquote><div><br></div><div>Cool. Let me propose how this might look with the new hashing, and mixing that is a bit more sound that xor and shift.</div>
<div><br></div><div>unsigned DenseMapInfo<SimpleValue>::getHashValue(SimpleValue Val) {</div><div> Instruction *Inst = Val.Inst;</div><div><br></div><div> if (isa<BinaryOperator>(Inst) && Inst->isCommutative()) {</div>
<div> Value *LHS = Inst->getOperand(0), *RHS = Inst->getOperand(1);</div><div> if (LHS > RHS) std::swap(LHS, RHS);</div><div> return (unsigned)hash_combine(LHS, RHS);</div><div> }</div><div> </div><div>
return (unsigned)hash_combine_range(value_op_iterator(Inst->op_begin()), value_op_iterator(Inst->op_end()));</div><div>}</div><div><br></div><div>Where we define 'value_op_iterator' to be an iterator wrapper around User::use_iterator which returns the Value* directly from operator* instead of returning the Use.</div>
<div><br></div><div>I wonder if it would be worthwhile to add this to User itself so that we have User::value_op_begin() and User::value_op_end().</div></div></div>
</blockquote></div><br></div></div><div>The new hashing stuff looks cool, but I have to admit I don't fully understand everything yet. I think I'll roll this out in two commits, one using the old hashing mechanism but with the changes as Chris described them, then another patch to switch to the new.</div>
</div></blockquote><div><br></div><div> I already described most of how it would work?</div><div><br></div><div>The version you have will have lots of problems with collisions I expect, and it is also much more code. Most of the code required for what I mentioned is to get a nicer iterator interface, which is something that other code will benefit from as well... I can actually implement the hashing stuff and send it back to you if it's not making sense how it would work.</div>
</div></div>