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

[Duncan Sands]

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.

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

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();