Consider all SCEVMulExpr operands for factoring in FactorOutConstant

[vhscampos]

Summary

In FactorOutConstant function, when the expression is a SCEVMulExpr, the code was considering only the first operand for factoring. As the comment in line 258 states, all operands should be tested until one suitable is found.

I also removed the TODO about SCEVSDivExpr since this class is not present anymore in LLVM.

[sanjoy]

lib/Analysis/ScalarEvolutionExpander.cpp

265

I'm not sure when you expect this to fire -- you should never have a SCEV mul expression with multiple constant operands, they should have all been folded into one (i.e. (2 * 3 * %val) => (6 * %val)). If that's not happening in some case, that's the real bug.

[vhscampos]

Okay. Thanks for the clarification. Should I create another patch just for the removal of lines 223-225?

[sanjoy]

In D26217#590831, @vhscampos wrote:

Okay. Thanks for the clarification. Should I create another patch just for the removal of lines 223-225?

Sure. Feel free to just check that patch in without pre-commit review.

lib/Analysis/ScalarEvolutionExpander.cpp

Value *SCEVExpander::InsertBinop(Instruction::BinaryOps Opcode,
 
   return BO;
 }
 
 /// FactorOutConstant - Test if S is divisible by Factor, using signed
 /// division. If so, update S with Factor divided out and return true.
 /// S need not be evenly divisible if a reasonable remainder can be
 /// computed.
-/// TODO: When ScalarEvolution gets a SCEVSDivExpr, this can be made
-/// unnecessary; in its place, just signed-divide Ops[i] by the scale and
-/// check to see if the divide was folded.
 static bool FactorOutConstant(const SCEV *&S, const SCEV *&Remainder,
                               const SCEV *Factor, ScalarEvolution &SE,
                               const DataLayout &DL) {
   // Everything is divisible by one.
   if (Factor->isOne())
     return true;
 
   // x/x == 1.
static bool FactorOutConstant(const SCEV *&S, const SCEV *&Remainder,
   }
 
   // In a Mul, check if there is a constant operand which is a multiple
   // of the given factor.
   if (const SCEVMulExpr *M = dyn_cast<SCEVMulExpr>(S)) {
     // Size is known, check if there is a constant operand which is a multiple
     // of the given factor. If so, we can factor it.
     const SCEVConstant *FC = cast<SCEVConstant>(Factor);
-    if (const SCEVConstant *C = dyn_cast<SCEVConstant>(M->getOperand(0)))
+    for (unsigned i = 0, e = M->getNumOperands(); i < e; ++i) {
+      if (const SCEVConstant *C = dyn_cast<SCEVConstant>(M->getOperand(i)))
         if (!C->getAPInt().srem(FC->getAPInt())) {
           SmallVector<const SCEV *, 4> NewMulOps(M->op_begin(), M->op_end());
-        NewMulOps[0] = SE.getConstant(C->getAPInt().sdiv(FC->getAPInt()));
+          NewMulOps[i] = SE.getConstant(C->getAPInt().sdiv(FC->getAPInt()));
           S = SE.getMulExpr(NewMulOps);
           return true;
         }
     }
+  }
 
   // In an AddRec, check if both start and step are divisible.
   if (const SCEVAddRecExpr *A = dyn_cast<SCEVAddRecExpr>(S)) {
     const SCEV *Step = A->getStepRecurrence(SE);
     const SCEV *StepRem = SE.getConstant(Step->getType(), 0);
     if (!FactorOutConstant(Step, StepRem, Factor, SE, DL))
       return false;
     if (!StepRem->isZero())