<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 class="im"><br>
On Oct 1, 2012, at 1:14 PM, Michael Ilseman <<a href="mailto:milseman@apple.com" 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>