[VPlan] Verify plan entry and exit blocks, set correct exit block.

[fhahn]

Summary

Both the entry and exit blocks of the top-region of a plan must be
VPBasicBlocks. They also must have no predecessors or successors
respectively.

This invariant was broken when splitting a block for sink-after. To fix
the issue, set the exit block of the region *after* sink-after is done.

[fhahn]

ping :)

[Ayal]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

9167–9168

Wonder if the above assert should also be taken care of by VPlan verify instead?

9238

Another way of fixing this would be to have split-blocks update Exit, when it updates VPBB?

llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp

169

The Entry and Exit blocks of any region in Plan must have no predecessors and successors, respectively, by definition; not only the top-most region, e.g., including internal replicating regions. Worth extending to check all regions in Plan? Entry and Exit blocks may themselves be regions, in general, so asserting that they be basic blocks should be restricted to the top-most region.

[fhahn]

Verify empty predecessors/successors for all region entries/exits.

[fhahn]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

9167–9168

Could do, the only missing piece would be the non-empty part. I think that makes sense here, but might be overly restrictive as general requirement?

9238

Yes, I can change it, but I think that be a bit more work.

llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp

169

Done, I updated the code to verify empty predecessors/successor for all region entries/exits.

[Ayal]

This is fine! Adding a follow-up thought.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

9238

(Continuing to reason about the above other way, independent of this patch which is fine - ) would it make sense to maintain a valid Region throughout, i.e., create it with an Entry and Exit upfront? That is, with a non-empty Entry following D111299, connected to an initially empty Exit, and where intra-loop code/blocks are to be introduced between VPBB and Exit (except for code seeking to be introduced in the latch/Exit..). Admittedly refers to D113182. Or would doing so interfere with how D111299 et al. seek to model/build the vector latch, preheader and exit blocks in VPlan.

[fhahn]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

9238

I don't think that would cause any major issues I think, just a potentially redundant block in the plan, if create a dedicated exit block up front.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

for (Instruction &I : BB->instructionsWithoutDebug()) {
                                     : "");
       }
     }
   }
 
   assert(isa<VPRegionBlock>(Plan->getEntry()) &&
          !Plan->getEntry()->getEntryBasicBlock()->empty() &&
          "entry block must be set to a VPRegionBlock having a non-empty entry "
          "VPBasicBlock");
-  cast<VPRegionBlock>(Plan->getEntry())->setExit(VPBB);
   RecipeBuilder.fixHeaderPhis();
 
   // ---------------------------------------------------------------------------
   // Transform initial VPlan: Apply previously taken decisions, in order, to
   // bring the VPlan to its final state.
   // ---------------------------------------------------------------------------
 
   // Apply Sink-After legal constraints.
   auto GetReplicateRegion = [](VPRecipeBase *R) -> VPRegionBlock * {
if (TargetRegion) {
       VPBlockUtils::disconnectBlocks(SplitPred, SplitBlock);
       VPBlockUtils::connectBlocks(SplitPred, SinkRegion);
       VPBlockUtils::connectBlocks(SinkRegion, SplitBlock);
       if (VPBB == SplitPred)
         VPBB = SplitBlock;
     }
   }
 
+  cast<VPRegionBlock>(Plan->getEntry())->setExit(VPBB);
+
   // Now that sink-after is done, move induction recipes for optimized truncates
   // to the phi section of the header block.
   for (VPWidenIntOrFpInductionRecipe *Ind : InductionsToMove)
     Ind->moveBefore(*HeaderVPBB, HeaderVPBB->getFirstNonPhi());
 
   // Adjust the recipes for any inloop reductions.
   adjustRecipesForReductions(VPBB, Plan, RecipeBuilder, Range.Start);
 

llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp

#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
         errs() << "after\n";
         std::prev(RecipeI)->dump();
 #endif
         return false;
       }
       RecipeI++;
     }
   }
+
+  const VPRegionBlock *TopRegion = cast<VPRegionBlock>(Plan.getEntry());
+  const VPBasicBlock *Entry = dyn_cast<VPBasicBlock>(TopRegion->getEntry());
+  if (!Entry) {
+    errs() << "VPlan entry block is not a VPBasicBlock\n";
+    return false;
+  }
+  const VPBasicBlock *Exit = dyn_cast<VPBasicBlock>(TopRegion->getExit());
+  if (!Exit) {
+    errs() << "VPlan exit block is not a VPBasicBlock\n";
+    return false;
+  }
+
+  for (const VPRegionBlock *Region :
+       VPBlockUtils::blocksOnly<const VPRegionBlock>(
+           depth_first(VPBlockRecursiveTraversalWrapper<const VPBlockBase *>(
+               Plan.getEntry())))) {
+    if (Region->getEntry()->getNumPredecessors() != 0) {
+      errs() << "region entry block has predecessors\n";
+      return false;
+    }
+    if (Region->getExit()->getNumSuccessors() != 0) {
+      errs() << "region exit block has successors\n";
+      return false;
+    }
+  }
   return true;
 }