[PATCH] D116320: [VPlan] Add prepareForExecution to set up live-ins (NFC).

[Ayal Zaks via Phabricator via llvm-commits]

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

This looks good to me, adding some optional suggestions.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7965
   State.CFG.PrevBB = ILV.createVectorizedLoopSkeleton();
-  State.TripCount = ILV.getOrCreateTripCount(nullptr);
   State.CanonicalIV = ILV.Induction;
   ILV.collectPoisonGeneratingRecipes(State);
----------------
Induction/CanonicalIV is another live-in that could be handled by prepareForExecution(), but soon D113223 will replace it and pass CanonicalIVStartValue to prepareForExecution() instead?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7970
 
+  BestVPlan.prepareForExecution(ILV.getOrCreateTripCount(nullptr), State);
+
----------------
Best placed right before calling BestVPlan.execute()?

Can alternatively have execute() call (a protected) prepareForExecution(), but in any case the caller needs to pass live-in Values separately from State.

Can alternatively be called prepareToExecute().


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8464
 
     if (TailFolded && CM.TTI.emitGetActiveLaneMask()) {
+      BlockMask = Builder.createNaryOp(VPInstruction::ActiveLaneMask,
----------------
Now that the comment is removed, so can the {} be removed...
Or set  `VPValue *TC = Plan->getOrCreateTripCount()`  analogous to the ICmpULE else case below.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:809
+  }
+}
+
----------------
Slightly more logical to first handle trip count and then backedge taken count?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2138
+  /// the tail.
+  VPValue *TripCount = nullptr;
+
----------------
Perhaps TripCount should appear first, before BackedgeTakenCount. In any case, may be good to mention how the two relate.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2174
+    if (TripCount)
+      delete TripCount;
     for (VPValue *Def : VPExternalDefs)
----------------
Can alternatively add TripCount et al. to VPValuesToFree when constructed, placing the remember-to-deallocate next to the allocation.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2207
+  }
+
   /// Mark the plan to indicate that using Value2VPValue is not safe any
----------------
Slightly more logical to list TripCount first and then BackedgeTakenCount?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116320/new/

https://reviews.llvm.org/D116320