!fixup address comments, thanks

Author: fhahn

llvm/lib/Transforms/Vectorize/VPlan.cpp

@@ -333,19 +333,19 @@ Value *VPTransformState::get(const VPValue *Def, bool NeedsScalar) {
     LastLane = 0;
   }
 
-  assert(IsSingleScalar && "must be a single-scalar at this point");
   // We need to construct the vector value for a single-scalar value by
   // broadcasting the scalar to all lanes.
-  auto *LastInst = cast<Instruction>(get(Def, LastLane));
+  // TODO: Replace by introducing Broadcast VPInstructions.
+  assert(IsSingleScalar && "must be a single-scalar at this point");
   // Set the insert point after the last scalarized instruction or after the
   // last PHI, if LastInst is a PHI. This ensures the insertelement sequence
   // will directly follow the scalar definitions.
   auto OldIP = Builder.saveIP();
+  auto *LastInst = cast<Instruction>(get(Def, LastLane));
   auto NewIP = isa<PHINode>(LastInst)
                    ? LastInst->getParent()->getFirstNonPHIIt()
                    : std::next(BasicBlock::iterator(LastInst));
   Builder.SetInsertPoint(&*NewIP);
-
   Value *VectorValue = GetBroadcastInstrs(ScalarValue);
   set(Def, VectorValue);
   Builder.restoreIP(OldIP);

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp

@@ -3542,13 +3542,12 @@ void VPlanTransforms::materializeBuildVectors(VPlan &Plan) {
   // VPReplicateRegion::execute(), via shouldPack().
   // TODO: materialize build vectors for replicating recipes in replicating
   // regions.
-  // TODO: materialize build vectors for VPInstructions.
   for (VPBasicBlock *VPBB :
        concat<VPBasicBlock *>(VPBBsOutsideLoopRegion, VPBBsInsideLoopRegion)) {
     for (VPRecipeBase &R : make_early_inc_range(*VPBB)) {
-      auto *DefR = dyn_cast<VPRecipeWithIRFlags>(&R);
-      if (!DefR || !isa<VPReplicateRecipe, VPInstruction>(DefR))
+      if (!isa<VPReplicateRecipe, VPInstruction>(&R))
         continue;
+      auto *DefR = dyn_cast<VPRecipeWithIRFlags>(&R);
       auto UsesVectorOrInsideReplicateRegion = [DefR, LoopRegion](VPUser *U) {
         VPRegionBlock *ParentRegion =
             cast<VPRecipeBase>(U)->getParent()->getParent();

llvm/lib/Transforms/Vectorize/VPlanTransforms.h

@@ -158,7 +158,7 @@ struct VPlanTransforms {
   /// Explicitly unroll \p Plan by \p UF.
   static void unrollByUF(VPlan &Plan, unsigned UF);
 
-  /// Replace each VPReplicateRecipe and replicating VPInstruction outside on
+  /// Replace each replicating VPReplicateRecipe and VPInstruction outside of
   /// any replicate region in \p Plan with \p VF single-scalar recipes.
   /// TODO: Also replicate VPReplicateRecipes inside replicate regions, thereby
   /// dissolving the latter.

llvm/lib/Transforms/Vectorize/VPlanUnroll.cpp

@@ -463,8 +463,9 @@ void VPlanTransforms::unrollByUF(VPlan &Plan, unsigned UF) {
   VPlanTransforms::removeDeadRecipes(Plan);
 }
 
-/// Create a single-scalar clone of \p DefR for lane \p Lane. Use \p
-/// Def2LaneDefs to look up scalar definitions for operands of \DefR.
+/// Create a single-scalar clone of \p DefR (must either be a VPReplicateRecipe
+/// or VPInstruction) for lane \p Lane. Use \p Def2LaneDefs to look up scalar
+/// definitions for operands of \DefR.
 static VPRecipeWithIRFlags *
 cloneForLane(VPlan &Plan, VPBuilder &Builder, Type *IdxTy,
              VPRecipeWithIRFlags *DefR, VPLane Lane,
@@ -503,10 +504,15 @@ cloneForLane(VPlan &Plan, VPBuilder &Builder, Type *IdxTy,
 
   VPRecipeWithIRFlags *New;
   if (auto *RepR = dyn_cast<VPReplicateRecipe>(DefR)) {
+    // TODO: have cloning of replicate recipes also provide the desired result
+    // coupled with setting its operands to NewOps (deriving IsSingleScalar and
+    // Mask from the operands?)
     New =
         new VPReplicateRecipe(RepR->getUnderlyingInstr(), NewOps,
                               /*IsSingleScalar=*/true, /*Mask=*/nullptr, *RepR);
   } else {
+    assert(isa<VPInstruction>(DefR) &&
+           "DefR must either be a VPReplicateRecipe or VPInstruction");
     New = DefR->clone();
     for (const auto &[Idx, Op] : enumerate(NewOps)) {
       New->setOperand(Idx, Op);
@@ -538,15 +544,15 @@ void VPlanTransforms::replicateByVF(VPlan &Plan, ElementCount VF) {
   SmallVector<VPRecipeBase *> ToRemove;
   for (VPBasicBlock *VPBB : VPBBsToUnroll) {
     for (VPRecipeBase &R : make_early_inc_range(*VPBB)) {
-      auto *DefR = dyn_cast<VPRecipeWithIRFlags>(&R);
-      if (!DefR || !isa<VPInstruction, VPReplicateRecipe>(DefR))
-        continue;
-      if ((isa<VPReplicateRecipe>(DefR) &&
-           cast<VPReplicateRecipe>(DefR)->isSingleScalar()) ||
-          (isa<VPInstruction>(DefR) &&
-           !cast<VPInstruction>(DefR)->doesGeneratePerAllLanes()))
+      if (!isa<VPInstruction, VPReplicateRecipe>(&R) ||
+          (isa<VPReplicateRecipe>(&R) &&
+           cast<VPReplicateRecipe>(&R)->isSingleScalar()) ||
+          (isa<VPInstruction>(&R) &&
+           !cast<VPInstruction>(&R)->doesGeneratePerAllLanes()))
         continue;
 
+      auto *DefR = dyn_cast<VPRecipeWithIRFlags>(&R);
+
       VPBuilder Builder(DefR);
       if (DefR->getNumUsers() == 0) {
         if (isa<StoreInst>(DefR->getUnderlyingInstr()) &&