[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)



================
Comment at: lib/Transforms/Vectorize/VPlan.cpp:42
+
+VPBasicBlock *VPBlockBase::getEntryBasicBlock() {
+  VPBlockBase *Block = this;
----------------
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.


================
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.
----------------
mkuper wrote:
> 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?
OK, sure; will do so using an Optional<Instance>.


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


================
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.
----------------
mkuper wrote:
> I don't understand this note. :-)
Some subsequent code dereferences getFirstInsertionPt(), in LV or in code called by LV, w/o checking if it might be the end of a block.


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


================
Comment at: lib/Transforms/Vectorize/VPlan.h:116
+
+  static unsigned NextOrdinal; ///< Unique ID generator.
+
----------------
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.


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


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


================
Comment at: lib/Transforms/Vectorize/VPlan.h:461
+  ~VPRegionBlock() {
+    if (Entry)
+      deleteCFG(Entry);
----------------
mkuper wrote:
> Is it possible not to have an Entry here?
> It seems like it shouldn't be, so this should probably be an assert.
It is conceivable for code motion to completely empty a Region from all its blocks.


https://reviews.llvm.org/D32871