[PATCH] D29641: [SLP] Fix for PR31847: Assertion failed: (isLoopInvariant(Operands[i], L) && "SCEVAddRecExpr operand is not loop-invariant!")

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1327
+      eraseInstruction(I);
+  });
+}
----------------
What do we gain from using std::for_each instead of basic range for loops here and in ~BoUpSLP()?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4736
+      }
+      return Builder.CreateSelect(Cmp, LHS, RHS, Name);
+    }
----------------
Would it be better to move the return into each min/max case, drop the RK_None case and just have the llvm_unreachable at the end of the function?

Similar question for initReductionOps / addReductionOps below


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4934
+        return Op;
+        break;
+      case RK_None:
----------------
Don't have a break after return - some compilers will shout at you.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4960
+        return Op;
         break;
       case RK_None:
----------------
Remove the break.


https://reviews.llvm.org/D29641