[PATCH] D21330: Loop vectorization with FP induction variables

[Adam Nemet via llvm-commits]

anemet added a comment.

You should probably also have a test for the case where there is not "fast" attribute on the step instruction but we can still vectorize with the hints.


================
Comment at: ../include/llvm/Transforms/Utils/LoopUtils.h:321-323
@@ -310,1 +320,5 @@
 
+  /// Returns true if the induction has unsafe algebra which requires a relaxed
+  /// floating-point model.
+  bool hasUnsafeAlgebra() { return UnsafeAlgebraInst != nullptr; }
+
----------------
This comment seems incorrect/misleading.  I think this flag "allows" a relaxed FP model not "requires" it.  Do you agree?

================
Comment at: ../include/llvm/Transforms/Utils/LoopUtils.h:328-329
@@ +327,4 @@
+
+  /// Returns binary opcode that advances induction variable.
+  Instruction::BinaryOps getInductionBinOp() const { return BinaryOp; }
+
----------------
I think you missed the part about "everywhere", i.e. making the member variable consistent with this name too: BinaryOp -> InductionOp

================
Comment at: ../include/llvm/Transforms/Utils/LoopUtils.h:343-344
@@ -320,1 +342,4 @@
   const SCEV *Step;
+  // Induction has unsafe algebra.
+  Instruction *UnsafeAlgebraInst;
+  // Original Induction opcode.
----------------
This comment is also ambiguous.  This is again the induction/step instruction *iff* the instruction allows for a relaxed FP model.

================
Comment at: ../lib/Transforms/Utils/LoopUtils.cpp:819-821
@@ +818,5 @@
+  // on function level.
+  Instruction *UAI = !BOp->hasUnsafeAlgebra() ? BOp : nullptr;
+  D = InductionDescriptor(StartValue, IK_FpInduction, Step, UAI,
+                          BOp->getOpcode());
+  return true;
----------------
Fair but it does not make sense to pass a different instructions for UAI and BO and the current interface allows for that.  At least then change the ctor to take a single instruction and derive UAI and BO from that.

================
Comment at: ../test/Transforms/LoopVectorize/float-induction.ll:12-13
@@ +11,4 @@
+; VEC4_INTERL1:       %[[VEC_INCR:.*]] = fmul fast float %{{.*}}, %[[INDEX]]
+; VEC4_INTERL1:       fsub fast float %init, %[[VEC_INCR]]
+; VEC4_INTERL1:       insertelement {{.*}} %[[FP_INC]]
+; VEC4_INTERL1-NEXT:  %[[FP_INC_BCST:.*]] = shufflevector
----------------
Where is the result of these guys used?  Would be good to check too.

================
Comment at: ../test/Transforms/LoopVectorize/float-induction.ll:16-17
@@ +15,4 @@
+; VEC4_INTERL1:       %[[VSTEP:.*]] = fmul fast <4 x float> %[[FP_INC_BCST]], <float 0.000000e+00, float 1.000000e+00, float 2.000000e+00, float 3.000000e+00>
+; VEC4_INTERL1-NEXT:  fsub fast <4 x float> {{.*}}, %[[VSTEP]]
+; VEC4_INTERL1:       store <4 x float> 
+
----------------
Why not also match the result of the fsub and check it in the store?


Repository:
  rL LLVM

https://reviews.llvm.org/D21330