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

[Michael Ilseman]

Sorry, just getting back around to this. I see what you're saying now, and here's a stab at implementing it. I'm sure the value_op_iterator class needs some cleanup or reorganization.



On Oct 2, 2012, at 4:40 PM, Chandler Carruth <chandlerc at google.com> wrote:

> On Tue, Oct 2, 2012 at 4:22 PM, Michael Ilseman <milseman at apple.com> wrote:
> 
> 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.
> 
>  I already described most of how it would work?
> 
> 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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121008/5ba4d90c/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ValueOpIterator.patch
Type: application/octet-stream
Size: 1857 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121008/5ba4d90c/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121008/5ba4d90c/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: EarlyCSE_hash_code.patch
Type: application/octet-stream
Size: 5296 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121008/5ba4d90c/attachment-0001.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121008/5ba4d90c/attachment-0002.html>