[PATCH] D28975: [LV] Introducing VPlan to model the vectorized code and drive its transformation

[Oren Ben Simhon via Phabricator via llvm-commits]

oren_ben_simhon added inline comments.


================
Comment at: lib/Transforms/Vectorize/VPlan.cpp:10
+//
+// This is the LLVM vectorization plan. It represents a candidate for
+// vectorization, allowing to plan and optimize how to vectorize a given loop
----------------
Please follow LLVM guidelines for file documentation:
http://llvm.org/docs/CodingStandards.html#file-headers


================
Comment at: lib/Transforms/Vectorize/VPlan.h:1
+//===- VPlan.h - Represent A Vectorizer Plan ------------------------------===//
+//
----------------
This file is very big and will probably gain some more weight in time. I think that some utility classes could be moved to a separate file.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:68
+class VPRecipeBase : public ilist_node_with_parent<VPRecipeBase, VPBasicBlock> {
+  friend class VPlanUtils;
+  friend class VPBasicBlock;
----------------
IMHO, VPlanUtils should not be a friend class, because you don't access any of its private/protected members. Instead use VPlanUtils statically.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:72
+private:
+  const unsigned char VRID; // Subclass identifier (for isa/dyn_cast)
+
----------------
use ///< for post member documentation.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:75
+  /// Each VPRecipe is contained in a single VPBasicBlock.
+  class VPBasicBlock *Parent;
+
----------------
I didn't follow the whole logic but are you sure that you need recipe parent?
Also i think that "owner" is a better descriptive of the member than parent.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:191
+
+  bool isScalarizing() {
+    return getVPRecipeID() == VPRecipeBase::VPScalarizeOneByOneSC;
----------------
The function should be const.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:310
+  /// List of predecessor blocks.
+  SmallVector<VPBlockBase *, 2> Predecessors;
+
----------------
I know that the initial size of SmallVectors has minimum effect, still i believe that the default size of the predecessors/Successors should be one.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:315
+
+  /// \brief Successor selector, null for zero or single successor blocks.
+  VPConditionBitRecipeBase *ConditionBitRecipe;
----------------
The \brief will have no effect because anyway the comment until the first dot will be considered as brief by Doxygen.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:514
+
+  /// \brief Return the underlying instruction list container.
+  ///
----------------
No need for the brief command.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:522
+  /// \brief Returns a pointer to a member of the instruction list.
+  static RecipeListTy VPBasicBlock::*getSublistAccess(VPRecipeBase *) {
+    return &VPBasicBlock::Recipes;
----------------
The function receives a parameter that it doesn't use.
Also it is static although it returns a non-static class member.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:592
+  /// Hold the Single Entry of the SESE region represented by the VPRegionBlock.
+  VPBlockBase *Entry;
+
----------------
Please consider using a single list/vector instead of two members Entry/Exit.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:714
+protected:
+  VPlan *Plan;
+
----------------
The plan is not used anywhere in the class.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:717
+  typedef iplist<VPRecipeBase> RecipeListTy;
+  RecipeListTy *getRecipes(VPBasicBlock *Block) { return &Block->Recipes; }
+
----------------
This function member is not referenced anywhere.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:720
+public:
+  VPlanUtils(VPlan *Plan) : Plan(Plan) {}
+
----------------
This class should be a singleton class that should not be initialized. As such the constructor should be private.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:723
+  ~VPlanUtils() {}
+
+  /// Create a unique name for a new VPlan entity such as a VPBasicBlock or
----------------
All functions that do not relate to a non-static member, should be static and const.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:795
+  /// \p Block is copied to be the parent of \p IfTrue and \p IfFalse.
+  void setTwoSuccessors(VPBlockBase *Block, VPConditionBitRecipeBase *R,
+                        VPBlockBase *IfTrue, VPBlockBase *IfFalse) {
----------------
I think that the function name should be more descriptive to reflect the nature of the function, for example "setConditionalSuccessors".


================
Comment at: lib/Transforms/Vectorize/VPlan.h:860
+// Provide specializations of GraphTraits to be able to treat a VPRegionBlock
+// as a graph of VPBlockBases...
+
----------------
Maybe use a doxygen style comment to all class documentations.


https://reviews.llvm.org/D28975