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

[Evan Cheng]

On Sep 24, 2012, at 9:46 AM, Michael Ilseman <milseman at apple.com> wrote:

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

I would strong advise against the new instruction approach. That's considered a definite no-no in LLVM. Do you have a good sense how much effort it is to implement expression numbering?

Evan

> 
> 
> 
>> 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
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits