[VPlan] Add unit test for createAndOptimizeReplicateRegions. NFC

[lukel97]

In #160449 some of the tests end up merging less blocks so we end up losing test coverage.
This adds a unit test to try and cover it elsewhere in a more predictable way, so it won't be influenced by e.g. whether or not the cost model decides to scalarize an instruction.

Two parts in createReplicateRegion/addReplicateRegions had to be relaxed to allow using a fake Instruction with no BasicBlock parent. I wasn't able to create a fake BasicBlock in the test without hitting iterator assertions.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-vectorizers

Author: Luke Lau (lukel97)

Changes

In #160449 some of the tests end up merging less blocks so we end up losing test coverage.
This adds a unit test to try and cover it elsewhere in a more predictable way, so it won't be influenced by e.g. whether or not the cost model decides to scalarize an instruction.

Two parts in createReplicateRegion/addReplicateRegions had to be relaxed to allow using a fake Instruction with no BasicBlock parent. I wasn't able to create a fake BasicBlock in the test without hitting iterator assertions.

---

Full diff: https://github.com/llvm/llvm-project/pull/164652.diff

3 Files Affected:

• (modified) llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp (+3-3)
• (modified) llvm/unittests/Transforms/Vectorize/CMakeLists.txt (+1)
• (added) llvm/unittests/Transforms/Vectorize/VPlanTransformsTest.cpp (+77)

diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
index 7bf8d83d2550c..90d8cfc791097 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
@@ -345,7 +345,6 @@ static VPRegionBlock *createReplicateRegion(VPReplicateRecipe *PredRecipe,
   Instruction *Instr = PredRecipe->getUnderlyingInstr();
   // Build the triangular if-then region.
   std::string RegionName = (Twine("pred.") + Instr->getOpcodeName()).str();
-  assert(Instr->getParent() && "Predicated instruction not in any basic block");
   auto *BlockInMask = PredRecipe->getMask();
   auto *MaskDef = BlockInMask->getDefiningRecipe();
   auto *BOMRecipe = new VPBranchOnMaskRecipe(
@@ -399,8 +398,9 @@ static void addReplicateRegions(VPlan &Plan) {
     VPBasicBlock *SplitBlock = CurrentBlock->splitAt(RepR->getIterator());
 
     BasicBlock *OrigBB = RepR->getUnderlyingInstr()->getParent();
-    SplitBlock->setName(
-        OrigBB->hasName() ? OrigBB->getName() + "." + Twine(BBNum++) : "");
+    SplitBlock->setName((OrigBB && OrigBB->hasName())
+                            ? OrigBB->getName() + "." + Twine(BBNum++)
+                            : "");
     // Record predicated instructions for above packing optimizations.
     VPRegionBlock *Region = createReplicateRegion(RepR, Plan);
     Region->setParent(CurrentBlock->getParent());
diff --git a/llvm/unittests/Transforms/Vectorize/CMakeLists.txt b/llvm/unittests/Transforms/Vectorize/CMakeLists.txt
index af111a29b90e5..3564bd140c971 100644
--- a/llvm/unittests/Transforms/Vectorize/CMakeLists.txt
+++ b/llvm/unittests/Transforms/Vectorize/CMakeLists.txt
@@ -14,6 +14,7 @@ add_llvm_unittest(VectorizeTests
   VPlanHCFGTest.cpp
   VPlanPatternMatchTest.cpp
   VPlanSlpTest.cpp
+  VPlanTransformsTest.cpp
   VPlanUncountableExitTest.cpp
   VPlanVerifierTest.cpp
   )
diff --git a/llvm/unittests/Transforms/Vectorize/VPlanTransformsTest.cpp b/llvm/unittests/Transforms/Vectorize/VPlanTransformsTest.cpp
new file mode 100644
index 0000000000000..ab94337d61e39
--- /dev/null
+++ b/llvm/unittests/Transforms/Vectorize/VPlanTransformsTest.cpp
@@ -0,0 +1,77 @@
+//===- llvm/unittests/Transforms/Vectorize/VPlanTransformsTest.cpp --------===//
+//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "../lib/Transforms/Vectorize/VPlanTransforms.h"
+#include "../lib/Transforms/Vectorize/VPlan.h"
+#include "VPlanTestBase.h"
+#include "gtest/gtest.h"
+
+namespace llvm {
+
+namespace {
+using VPlanTransformsTest = VPlanTestBase;
+
+// Test we create and merge replicate regions:
+// VPBB1:
+//   REPLICATE %Rep0 = add
+//   REPLICATE %Rep1 = add
+//   REPLICATE %Rep2 = add
+// No successors
+//
+// ->
+//
+// <xVFxUF> pred.add: {
+//   pred.add.entry:
+//     BRANCH-ON-MASK %Mask
+//   Successor(s): pred.add.if, pred.add.continue
+//
+//   pred.add.if:
+//     REPLICATE %Rep0 = add
+//     REPLICATE %Rep1 = add
+//     REPLICATE %Rep2 = add
+//   Successor(s): pred.add.continue
+//
+//   pred.add.continue:
+//   No successors
+// }
+TEST_F(VPlanTransformsTest, createAndOptimizeReplicateRegions) {
+  VPlan &Plan = getPlan();
+
+  IntegerType *Int32 = IntegerType::get(C, 32);
+  auto *AI = BinaryOperator::CreateAdd(PoisonValue::get(Int32),
+                                       PoisonValue::get(Int32));
+
+  auto *VPBB0 = Plan.getEntry();
+  auto *VPBB1 = Plan.createVPBasicBlock("VPBB1");
+  VPRegionBlock *R1 = Plan.createVPRegionBlock(VPBB1, VPBB1, "R1");
+  VPBlockUtils::connectBlocks(VPBB0, R1);
+  auto *Mask = new VPInstruction(0, {});
+  auto *Rep0 = new VPReplicateRecipe(AI, {}, false, Mask);
+  auto *Rep1 = new VPReplicateRecipe(AI, {}, false, Mask);
+  auto *Rep2 = new VPReplicateRecipe(AI, {}, false, Mask);
+  VPBB1->appendRecipe(Rep0);
+  VPBB1->appendRecipe(Rep1);
+  VPBB1->appendRecipe(Rep2);
+
+  VPlanTransforms::createAndOptimizeReplicateRegions(Plan);
+
+  auto *Replicator = cast<VPRegionBlock>(R1->getEntry()->getSingleSuccessor());
+  EXPECT_TRUE(Replicator->isReplicator());
+  auto *ReplicatorEntry = cast<VPBasicBlock>(Replicator->getEntry());
+  EXPECT_EQ(ReplicatorEntry->size(), 1u);
+  EXPECT_TRUE(isa<VPBranchOnMaskRecipe>(ReplicatorEntry->front()));
+  auto *ReplicatorIf = cast<VPBasicBlock>(ReplicatorEntry->getSuccessors()[0]);
+  EXPECT_EQ(ReplicatorIf->size(), 3u);
+  EXPECT_EQ(ReplicatorEntry->getSuccessors()[1],
+            ReplicatorIf->getSingleSuccessor());
+  EXPECT_EQ(ReplicatorIf->getSingleSuccessor(), Replicator->getExiting());
+}
+
+} // namespace
+} // namespace llvm

[fhahn]

Could we avoid this by updating the IR tests to use explicitly predicated blocks, instead of relying on tail-folding?

The C++ unit tests are a bit more cumbersome to update, so if we could avoid that it would be great

[lukel97]

Could we avoid this by updating the IR tests to use explicitly predicated blocks, instead of relying on tail-folding?

The C++ unit tests are a bit more cumbersome to update, so if we could avoid that it would be great

I originally tried this but there's a few test cases in first-order-recurrence-sink-replicate-region.ll like @sink_replicate_region_1 that can't be converted to explicitly predicated blocks because the FOR can't be predicated. But for the tests that are affected by #160449 we can explicitly predicate them, if you're ok with only some of them being updated.

[lukel97]

Could we avoid this by updating the IR tests to use explicitly predicated blocks, instead of relying on tail-folding?
The C++ unit tests are a bit more cumbersome to update, so if we could avoid that it would be great

I originally tried this but there's a few test cases in first-order-recurrence-sink-replicate-region.ll like @sink_replicate_region_1 that can't be converted to explicitly predicated blocks because the FOR can't be predicated. But for the tests that are affected by #160449 we can explicitly predicate them, if you're ok with only some of them being updated.

Opened up #164934 to do this