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

[Chandler Carruth]

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/20121002/793a5add/attachment.html>