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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal marked 3 inline comments as done.
Ayal added a comment.

> (Addressing review comments; To be completed)

Done. Will upload updated version shortly.



================
Comment at: lib/Transforms/Vectorize/VPlan.cpp:42
+
+VPBasicBlock *VPBlockBase::getEntryBasicBlock() {
+  VPBlockBase *Block = this;
----------------
Ayal wrote:
> mkuper wrote:
> > Any chance to make the const/non-const versions, here and below, share implementation? It seems like it ought to be possible.
> Not sure how, w/o casting away const.
> 
> We can drop the non-const getEntryBasicBlock() as only its const version is currently used, but both versions of getExitBasicBlock() are used.
Leaving them as is.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:116
+
+  static unsigned NextOrdinal; ///< Unique ID generator.
+
----------------
Ayal wrote:
> mkuper wrote:
> > 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?)
> Yes, UID is used for printing only, including the Name. Thinking of having the Planner keep track of this ordinal.
Assigning the IDs on the fly in the printer, instead.


================
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; }
+
----------------
mkuper wrote:
> Does this strictly need to be public? It looks like it would only be used in the classof of subclasses. Can it be protected?
It needs to be public. A method (classof) of a derived class cannot invoke a protected method on an object of the base class (not "this"). 


================
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.
----------------
mkuper wrote:
> 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?
Added the following definition to clarify the terminology:
  /// An Ancestor of a block B is any block containing B, including B itself.
So it's either an "enclosing region" or the block itself, which in turn may be a region or a basic-block.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:389
+  /// if you want to modify it.
+  const RecipeListTy &getRecipeList() const { return Recipes; }
+  RecipeListTy &getRecipeList() { return Recipes; }
----------------
mkuper wrote:
> 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?
Yes we can.
(May sound old-fashioned ;-)


================
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;
----------------
mkuper wrote:
> This isn't used anywhere either - and even if it were, the whole thing looks odd.
This odd looking thing is part of the ilist_node_with_parent contract, used by getPrevNode() and getNextNode().


================
Comment at: lib/Transforms/Vectorize/VPlan.h:496
+  /// Holds the VFs applicable to this VPlan.
+  std::set<unsigned> VFs;
+
----------------
mkuper wrote:
> Maybe a SmallSet?
Initially wanted to iterate over the VFs, but that's not needed now.
Changed to SmallSet, and also replaced getVFs() with hasVF(unsigned).


================
Comment at: lib/Transforms/Vectorize/VPlan.h:519
+
+  void removeVF(unsigned VF) { VFs.erase(VF); }
+
----------------
mkuper wrote:
> Is this functionality we want?
Possibly, but not now, removed.


================
Comment at: lib/Transforms/Vectorize/VPlan.h:521
+
+  std::set<unsigned> &getVFs() { return VFs; }
+  const std::set<unsigned> &getVFs() const { return VFs; }
----------------
mkuper wrote:
> I don't think we want a non-const version of this.
We can do w/o both versions, by only checking hasVF(unsigned).


================
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);
----------------
mkuper wrote:
> Should this be static? Or maybe even a free function in the implementation?
Yes, this should be static. It's offloading the last part of VPlan::execute(), so good to keep adjacent.


https://reviews.llvm.org/D32871