The accumulator tail recursion transform claims to work for any associative

operation, but the way it's implemented requires the operation to also be
commutative.  So add a check for commutativity (and tweak the corresponding
comments).  This makes no difference in practice since every associative
LLVM instruction is also commutative!  Here's an example to show the need
for commutativity: the accum_recursion.ll testcase calculates the factorial
function.  Before the transformation the result of a call is
  ((((1*1)*2)*3)...)*x
while afterwards it is
  (((1*x)*(x-1))...*2)*1
which clearly requires both associativity and commutativity of * to be equal
to the original.


git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@108056 91177308-0d34-0410-b5e6-96231b3b80d8

lib/Transforms/Scalar/TailRecursionElimination.cpp

@@ -16,9 +16,9 @@
 //     transformation from taking place, though currently the analysis cannot
 //     support moving any really useful instructions (only dead ones).
 //  2. This pass transforms functions that are prevented from being tail
-//     recursive by an associative expression to use an accumulator variable,
+//     recursive by an associative and commutative expression to use an
-//     thus compiling the typical naive factorial or 'fib' implementation into
+//     accumulator variable, thus compiling the typical naive factorial or
-//     efficient code.
+//     'fib' implementation into efficient code.
 //  3. TRE is performed if the function returns void, if the return
 //     returns the result returned by the call, or if the function returns a
 //     run-time constant on all exits from the function.  It is possible, though
@@ -302,9 +302,9 @@ static Value *getCommonReturnValue(ReturnInst *IgnoreRI, CallInst *CI) {
 ///
 Value *TailCallElim::CanTransformAccumulatorRecursion(Instruction *I,
                                                       CallInst *CI) {
-  if (!I->isAssociative()) return 0;
+  if (!I->isAssociative() || !I->isCommutative()) return 0;
   assert(I->getNumOperands() == 2 &&
-         "Associative operations should have 2 args!");
+         "Associative/commutative operations should have 2 args!");
 
   // Exactly one operand should be the result of the call instruction...
   if ((I->getOperand(0) == CI && I->getOperand(1) == CI) ||
@@ -369,11 +369,11 @@ bool TailCallElim::ProcessReturningBlock(ReturnInst *Ret, BasicBlock *&OldEntry,
       return false;
   }
 
-  // If we are introducing accumulator recursion to eliminate associative
+  // If we are introducing accumulator recursion to eliminate operations after
-  // operations after the call instruction, this variable contains the initial
+  // the call instruction that are both associative and commutative, the initial
-  // value for the accumulator.  If this value is set, we actually perform
+  // value for the accumulator is placed in this variable.  If this value is set
-  // accumulator recursion elimination instead of simple tail recursion
+  // then we actually perform accumulator recursion elimination instead of
-  // elimination.
+  // simple tail recursion elimination.
   Value *AccumulatorRecursionEliminationInitVal = 0;
   Instruction *AccumulatorRecursionInstr = 0;
 
@@ -384,9 +384,9 @@ bool TailCallElim::ProcessReturningBlock(ReturnInst *Ret, BasicBlock *&OldEntry,
   for (BBI = CI, ++BBI; &*BBI != Ret; ++BBI)
     if (!CanMoveAboveCall(BBI, CI)) {
       // If we can't move the instruction above the call, it might be because it
-      // is an associative operation that could be tranformed using accumulator
+      // is an associative and commutative operation that could be tranformed
-      // recursion elimination.  Check to see if this is the case, and if so,
+      // using accumulator recursion elimination.  Check to see if this is the
-      // remember the initial accumulator value for later.
+      // case, and if so, remember the initial accumulator value for later.
       if ((AccumulatorRecursionEliminationInitVal =
                              CanTransformAccumulatorRecursion(BBI, CI))) {
         // Yes, this is accumulator recursion.  Remember which instruction
@@ -483,7 +483,7 @@ bool TailCallElim::ProcessReturningBlock(ReturnInst *Ret, BasicBlock *&OldEntry,
     }
 
     // Add an incoming argument for the current block, which is computed by our
-    // associative accumulator instruction.
+    // associative and commutative accumulator instruction.
     AccPN->addIncoming(AccRecInstr, BB);
 
     // Next, rewrite the accumulator recursion instruction so that it does not