[PATCH] D32871: [LV] Using VPlan to model the vectorized code and drive its transformation

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added a comment.

Sorry it's taking me so long to get to this.
I've only looked at parts of the code, more forthcoming in the next few days. :-)



================
Comment at: lib/Transforms/Vectorize/VPlan.cpp:42
+
+VPBasicBlock *VPBlockBase::getEntryBasicBlock() {
+  VPBlockBase *Block = this;
----------------
Any chance to make the const/non-const versions, here and below, share implementation? It seems like it ought to be possible.


================
Comment at: lib/Transforms/Vectorize/VPlan.cpp:190
+    for (unsigned Lane = 0, VF = State->VF; Lane < VF; ++Lane) {
+      Instance = {Part, Lane};
+      // Visit the VPBlocks connected to \p this, starting from it.
----------------
This is extremely confusing. You're modifying what looks like a local variable to actually change the State->Instance for the State that will get passed to Block->execute(). Could you rewrite in a way that makes it clear what's going on?


================
Comment at: lib/Transforms/Vectorize/VPlan.cpp:209
+  BasicBlock *VectorLatchBB = VectorHeaderBB;
+  auto CurrIP = State->Builder.saveIP();
+
----------------
Why do we need to save/restore the builder IP?


================
Comment at: lib/Transforms/Vectorize/VPlan.cpp:218
+  // Temporarily terminate with unreachable until CFG is rewired.
+  // Note: this asserts the generated code's assumption that
+  // getFirstInsertionPt() can be dereferenced into an Instruction.
----------------
I don't understand this note. :-)


================
Comment at: lib/Transforms/Vectorize/VPlan.h:114
+private:
+  const unsigned char VBID; ///< Subclass identifier (for isa/dyn_cast).
+
----------------
I think naming this SubclassID (like Value does) or something similar would be clearer. Same for VRID.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:116
+
+  static unsigned NextOrdinal; ///< Unique ID generator.
+
----------------
This look not at all thread-safe, and it seems like the only thing that actually uses the UIDs is the printer.
Perhaps assign the IDs on the fly in the printer?
(Does anything else in LLVM do it this way?)


================
Comment at: lib/Transforms/Vectorize/VPlan.h:173
+  typedef SmallVectorImpl<VPBlockBase *> VPBlocksTy;
+  typedef const SmallVectorImpl<VPBlockBase *> const_VPBlocksTy;
+
----------------
I think this should be ConstVPBlaocksTy or something of that sort.
(Or, just use const VPBlocksTy?)


================
Comment at: lib/Transforms/Vectorize/VPlan.h:186
+  /// for any other purpose, as the values may change as LLVM evolves.
+  unsigned getVPBlockID() const { return VBID; }
+
----------------
Does this strictly need to be public? It looks like it would only be used in the classof of subclasses. Can it be protected?


================
Comment at: lib/Transforms/Vectorize/VPlan.h:188
+
+  const class VPRegionBlock *getParent() const { return Parent; }
+
----------------
You can drop the "class" here and everywhere below.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:222
+
+  /// \return the closest ancestor starting from "this", which has successors.
+  /// Returns the root ancestor if all ancestors have no successors.
----------------
The terminology here is somewhat confusing, since it's not immediately clear what the difference between an "ancestor" and a "predecessor" is. Maybe "enclosing region" instead of ancestor?


================
Comment at: lib/Transforms/Vectorize/VPlan.h:306
+private:
+  const unsigned char VRID; ///< Subclass identifier (for isa/dyn_cast).
+
----------------
Same as above.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:313
+  /// An enumeration for keeping track of the concrete subclass of VPRecipeBase
+  /// that are actually instantiated. Values of this enumeration are kept in the
+  /// VRID field of the VPRecipeBase objects. They are used for concrete type
----------------
are -> is?


================
Comment at: lib/Transforms/Vectorize/VPlan.h:316
+  /// identification.
+  typedef enum {
+    VPBranchOnMaskSC,
----------------
I'm not sure about the type safety of this. Is this how we do it elsewhere?


================
Comment at: lib/Transforms/Vectorize/VPlan.h:389
+  /// if you want to modify it.
+  const RecipeListTy &getRecipeList() const { return Recipes; }
+  RecipeListTy &getRecipeList() { return Recipes; }
----------------
Do we want the const accessor? IIUC, if you're not modifying it, you should be using the forwarding methods.
In any case, it looks like neither of these methods is used right now. Can we remove both?


================
Comment at: lib/Transforms/Vectorize/VPlan.h:393
+  /// \brief Returns a pointer to a member of the instruction list.
+  static RecipeListTy VPBasicBlock::*getSublistAccess(VPRecipeBase *) {
+    return &VPBasicBlock::Recipes;
----------------
This isn't used anywhere either - and even if it were, the whole thing looks odd.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:461
+  ~VPRegionBlock() {
+    if (Entry)
+      deleteCFG(Entry);
----------------
Is it possible not to have an Entry here?
It seems like it shouldn't be, so this should probably be an assert.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:496
+  /// Holds the VFs applicable to this VPlan.
+  std::set<unsigned> VFs;
+
----------------
Maybe a SmallSet?


================
Comment at: lib/Transforms/Vectorize/VPlan.h:519
+
+  void removeVF(unsigned VF) { VFs.erase(VF); }
+
----------------
Is this functionality we want?


================
Comment at: lib/Transforms/Vectorize/VPlan.h:521
+
+  std::set<unsigned> &getVFs() { return VFs; }
+  const std::set<unsigned> &getVFs() const { return VFs; }
----------------
I don't think we want a non-const version of this.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:531
+  /// that was created between it and the latch block, inclusive.
+  void updateDominatorTree(class DominatorTree *DT, BasicBlock *LoopPreHeaderBB,
+                           BasicBlock *LoopLatchBB);
----------------
Should this be static? Or maybe even a free function in the implementation?


https://reviews.llvm.org/D32871