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

[Michael Ilseman]

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

Overflow should be orthogonal to commutativity. I think you're right in that hashing should respect it, so I'll mix in the overflow bits for the hash.

> 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?


I'm not sure what's meant here. What code will be simplified by this? CmpInsts are a little different in that even "non-commutative" compares can be commuted if the predicate is swapped. In my patches, I canonicalize based on address (and swap the predicate accordingly).

Thanks for the feedback!


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

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