[llvm-commits] [PATCH] Make EarlyCSE understand commutativity

[Chandler Carruth]

On Mon, Oct 1, 2012 at 10:51 PM, Chris Lattner <clattner at apple.com> wrote:

>
> On Oct 1, 2012, at 1:14 PM, Michael Ilseman <milseman at apple.com> wrote:
> > 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.
>
> 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.
>

Ok, this is making more sense now. Sorry for my confusion earlier.


> What I'm suggesting is that the hashing code can look like (psuedo code
> obviously):
>
> unsigned DenseMapInfo<SimpleValue>::getHashValue(SimpleValue Val) {
>   Instruction *Inst = Val.Inst;
>
>   // Hash in all of the operands as pointers.
>   bool Swapped = false;
>   if (isa<BinaryOperator>(Inst) && Inst->isCommutative() &&
> Inst->getOperand(0) > Inst->getOperand(1)) {
>     Inst->swapOperands();
>     Swapped = true;
>   }
>
>   unsigned Res = 0;
>   for (unsigned i = 0, e = Inst->getNumOperands(); i != e; ++i)
>     Res ^= getHash(Inst->getOperand(i)) << (i & 0xF);
>
>  ...
>
>   // Mix in the opcode.
>   Res = (Res << 1) ^ Inst->getOpcode();
>
>   if (Swapped) Inst->swapOperands();
>   return Res;
> }
>
> Similarly for compares.
>
> 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.
>

Cool. Let me propose how this might look with the new hashing, and mixing
that is a bit more sound that xor and shift.

unsigned DenseMapInfo<SimpleValue>::getHashValue(SimpleValue Val) {
  Instruction *Inst = Val.Inst;

  if (isa<BinaryOperator>(Inst) && Inst->isCommutative()) {
    Value *LHS = Inst->getOperand(0), *RHS = Inst->getOperand(1);
    if (LHS > RHS) std::swap(LHS, RHS);
    return (unsigned)hash_combine(LHS, RHS);
  }

  return (unsigned)hash_combine_range(value_op_iterator(Inst->op_begin()),
value_op_iterator(Inst->op_end()));
}

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.

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().
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121001/83697975/attachment.html>