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

[Chandler Carruth]

This is definitely in the direction I was thinking.

The ValueOpIterator patch looks generally good with a few tweaks: add
doxygen comments and fix the 80-column issues I think I spotted...

The CSE patch seems largely good, except for this bit:

+    // Check overflow attributes
+    if (isa<OverflowingBinaryOperator>(LHSBinOp)) {
+      assert(isa<OverflowingBinaryOperator>(RHSBinOp)
+             && "same opcode, but different operator type?");
+      if (LHSBinOp->hasNoUnsignedWrap() != RHSBinOp->hasNoUnsignedWrap() ||
+          LHSBinOp->hasNoSignedWrap() != RHSBinOp->hasNoSignedWrap())
+        return false;
+    }

If this impacts commutativity, both the hashing and equality testing should
respect it.

If you can't lift this logic into the operator (or instruction), maybe
factor all of the commutatitivy logic? Make a static isCommutativeOp()
static function, and then you can even fold the CmpInst logic into it as
well?


On Mon, Oct 8, 2012 at 9:23 PM, Michael Ilseman <milseman at apple.com> wrote:

> 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/19531114/attachment.html>