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

[Michael Ilseman]

On Sep 22, 2012, at 3:01 AM, Duncan Sands <baldrick at free.fr> wrote:

> Hi Michael,
> 
>>> The attached patch and tests alter EarlyCSE to also check for commuted values. This allows EarlyCSE to replace:
>>> 
>>> t1 = fadd a, b
>>> t2 = fadd b, a
>>> 
>>> with:
>>> t1 = fadd a, b
> 
> -ENOTESTCASE.
> 

Tests cases were in the commute.ll file that was attached along with the patch.

>> @@ -397,8 +398,30 @@ bool EarlyCSE::processNode(DomTreeNode *Node) {
>> 
>>     // If this is a simple instruction that we can value number, process it.
>>     if (SimpleValue::canHandle(Inst)) {
>> +
>>       // See if the instruction has an available value.  If so, use it.
>> -      if (Value *V = AvailableValues->lookup(Inst)) {
>> +      Value *V = AvailableValues->lookup(Inst);
>> +
>> +      // If this is a commutative op, also see if its commuted value is
>> +      // available.
>> +      if (!V) {
>> +        if (BinaryOperator *BinOp = dyn_cast<BinaryOperator>(Inst)) {
>> +          if (BinOp->isCommutative()) {
>> +            BinaryOperator *Commuted = cast<BinaryOperator>(BinOp->clone());
>> +            Commuted->swapOperands();
>> +            V = AvailableValues->lookup(Commuted);
>> +            ilist_node_traits<Instruction>::deleteNode(Commuted);
> 
> urgh, I really don't like creating an instruction just to look it up, then
> deleting it again.  Can you make the lookup logic more flexible instead?
> By the way, how GVN takes care of commutativity is: when computing the value
> number for an instruction, it sorts the operands of commutative instructions
> (its sorts the operands according to their value number) before calculating
> a value number from them, ensuring that if two commutative instructions are
> the same except for a different operand order then they get the same value
> number.  I don't know how EarlyCSE works, but maybe something along these lines
> would also be possible?
> 

Thanks for the feedback!

I also don't like making a new instruction, but considered it the lesser of evils. Essentially, EarlyCSE uses a DenseMap over the instructions, but overloading hash to hash the instruction itself rather than its address. It also overloads isEqual to call Instruction::isIdenticalTo.

As far as changing the lookup mechanism, that would involve both sorting operand hashes on getHashValue, and extending Instruction::isIdenticalToWhenDefined to also calculate equality modulo commutativity (or, essentially inlining its body into the overloaded isEqual function rather than making the call). I don't know what is more desirable, but the original patch to EarlyCSE at least kept it very localized and would only trigger when a commutative op was encountered.

Another option is to restructure EarlyCSE to do expression numbering similarly to GVN, as you mentioned. Again, I don't know if this is more desirable over a localized call to clone(), as this would require a re-encapsulation of Instruction's data that would affect all of EarlyCSE.



> Ciao, Duncan.
> 
>> +          }
>> +        } else if (CmpInst *CI = dyn_cast<CmpInst>(Inst)) {
>> +          // For CmpInst, swapping will also correct the predicate
>> +          CmpInst *Commuted = cast<CmpInst>(CI->clone());
>> +          Commuted->swapOperands();
>> +          V = AvailableValues->lookup(Commuted);
>> +          ilist_node_traits<Instruction>::deleteNode(Commuted);
>> +        }
>> +      }
>> +
>> +      if (V) {
>>         DEBUG(dbgs() << "EarlyCSE CSE: " << *Inst << "  to: " << *V << '\n');
>>         Inst->replaceAllUsesWith(V);
>>         Inst->eraseFromParent();
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits