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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
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);
----------------
Ayal wrote:
> Induction/CanonicalIV is another live-in that could be handled by prepareForExecution(), but soon D113223 will replace it and pass CanonicalIVStartValue to prepareForExecution() instead?
Yes, `CanonicalIV` won't be needed after D113223 and I'll updated D113223 to use `prepareToExecute` to handle `CanonicalIVStartValue`


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7970
 
+  BestVPlan.prepareForExecution(ILV.getOrCreateTripCount(nullptr), State);
+
----------------
Ayal wrote:
> 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().
moved and renamed in the committed version


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2138
+  /// the tail.
+  VPValue *TripCount = nullptr;
+
----------------
Ayal wrote:
> Perhaps TripCount should appear first, before BackedgeTakenCount. In any case, may be good to mention how the two relate.
mentioned that `BTC` is `TripCount -1` in the comment for `BTC`.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116320