[PATCH] D12284: [LoopVectorize] Extract InductionInfo into a helper class

[Adam Nemet via llvm-commits]

anemet accepted this revision.
anemet added a comment.
This revision is now accepted and ready to land.

LGTM, a few things for you to consider:


================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:211-213
@@ +210,5 @@
+
+private:
+  /// Private constructor - use \c isInductionPHI.
+  InductionDescriptor(Value *Start, InductionKind K, ConstantInt *Step);
+public:
----------------
Move this to the other private members?  Also s/use/used by/

================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:215
@@ +214,3 @@
+public:
+  /// Default constructor made public for convenience.
+  InductionDescriptor()
----------------
That's a pretty strange comment.  It's made public so that isInductionPHI can do the actual initialization.

I actually don't think that's a very nice interface.  If you want something like a factory method to create InductionDescriptors then I would name it something like createFromPHI.  Then have an isInduction() member to check that the descriptor is an induction variable (!IK_NoInduction).

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:520-521
@@ -473,2 +519,4 @@
 
+  Value *StartValue =
+    Phi->getIncomingValueForBlock(AR->getLoop()->getLoopPreheader());
   const SCEV *Step = AR->getStepRecurrence(*SE);
----------------
I agree that this is the same loop whose header the phi is located in but I think it would be good to assert that.


Repository:
  rL LLVM

http://reviews.llvm.org/D12284