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

[Michael Ilseman]

On Oct 1, 2012, at 11:17 PM, Chandler Carruth <chandlerc at google.com> wrote:

> 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().

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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121002/79f5344c/attachment.html>