[PATCH] D22918: [Loop Vectorizer] Support predication of div/rem

[Adam Nemet via llvm-commits]

anemet added inline comments.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3385-3386
@@ -3382,1 +3384,4 @@
+/// set if the result needs to be inserted and/or extracted from vectors.
+/// If the instruction is also to be predicated, insertion cost is added the
+/// cost of the PHI node required.
 static unsigned getScalarizationOverhead(Type *Ty, bool Insert, bool Extract,
----------------
There is something wrong with this sentence.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3388
@@ -3382,2 +3387,3 @@
 static unsigned getScalarizationOverhead(Type *Ty, bool Insert, bool Extract,
+                                         bool Predicate,
                                          const TargetTransformInfo &TTI) {
----------------
Predicate->Predicated

Same in the other functions.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3416-3417
@@ -3399,1 +3415,4 @@
 
+/// \brief Estimate the overhead of scalarizing an instruction based on the
+/// types of its operand and return value.
+static unsigned getScalarizationOverhead(SmallVectorImpl<Type *>& OpTys,
----------------
Explain in the comment how this guys is different from the previous one.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3431-3432
@@ +3430,4 @@
+
+/// \brief Estimate the overhead of scalarizing an instruction based on the
+/// types of its operand and return value.
+static unsigned getScalarizationOverhead(Instruction *I, Type* RetTy,
----------------
Same here.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3433
@@ +3432,3 @@
+/// types of its operand and return value.
+static unsigned getScalarizationOverhead(Instruction *I, Type* RetTy,
+                                         unsigned VF, bool Predicate,
----------------
Type* -> Type *

Did you run clang-format on the diff?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4105
@@ +4104,3 @@
+    for (Use &Op : I->operands()) {
+      ExtractElementInst *OpInst = dyn_cast<ExtractElementInst>(&*Op);
+      if (!OpInst)
----------------
auto *

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4108-4115
@@ +4107,10 @@
+        continue;
+      bool CanSinkToUse = true;
+      for (User *U : OpInst->users()) {
+        if (U != &*I) {
+          // The extractelement is feeding another instruction - give up.
+          CanSinkToUse = false;
+          break;
+        }
+      }
+      if (CanSinkToUse)
----------------
Is this any different than OpInst->hasOneUse?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4135-4138
@@ +4134,6 @@
+        IncomingFalse = IEI->getOperand(0); // the unmodified vector
+      } else {
+        // Phi node will be created for the scalar predicated instruction.
+        IncomingTrue = &*I;
+        IncomingFalse = UndefValue::get(I->getType());
+      }
----------------
Is there a test for the non-insertelt case?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4138
@@ +4137,3 @@
+        IncomingTrue = &*I;
+        IncomingFalse = UndefValue::get(I->getType());
+      }
----------------
Why is the undef correct here?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4347
@@ +4346,3 @@
+  Value *Op2 = I.getOperand(1);
+  ConstantInt *CInt = dyn_cast<ConstantInt>(Op2);
+  return !CInt || CInt->isZero();
----------------
auto *


https://reviews.llvm.org/D22918