[PATCH] D21330: Loop vectorization with FP induction variables

[Michael Kuperstein via llvm-commits]

mkuper added inline comments.

================
Comment at: ../include/llvm/Transforms/Utils/LoopUtils.h:274
@@ -273,1 +273,3 @@
+    : StartValue(nullptr), IK(IK_NoInduction), Step(nullptr),
+    UnsafeAlgebraInst(nullptr), BinaryOp((Instruction::BinaryOps)0) {}
 
----------------
I think Instruction::BinaryOpsEnd may be better for an explicitly invalid BinaryOp. Not sure that's a good choice, but pretty sure 0 isn't.

================
Comment at: ../lib/Transforms/Utils/LoopUtils.cpp:787
@@ +786,3 @@
+
+  if (!dyn_cast<BinaryOperator>(BEValue))
+    return false;
----------------
I think what sanjoy meant was:

```
BinaryOperator *BOp = dyn_cast<BinaryOperator>(BEValue);
if (!BOp)
  return false;
```

================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:4100
@@ +4099,3 @@
+    // canonical one.
+    assert(P != OldInduction && "Main induction can be integer only");
+
----------------
Main -> Primary (I think we use that consistently)

================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:6402
@@ +6401,3 @@
+
+  if (Ty->isFloatingPointTy()) {
+    Constant *C = ConstantFP::get(Ty, (double)StartIdx);
----------------
Can you add a test for this? All of the tests you added force UF == 1.

================
Comment at: ../lib/Transforms/Vectorize/LoopVectorize.cpp:6405
@@ +6404,3 @@
+
+    // Floating point operations had to be 'fast' to enable the unrolling.
+    FastMathFlags Flags;
----------------
Are you sure about this? I mean, it's true for vectorizing, but is it true here as well?
(I'm not saying it isn't, just making sure this is intentional)


Repository:
  rL LLVM

https://reviews.llvm.org/D21330