[VPlan] Remove CanonicalIV when dissolving loop regions (NFC).

[fhahn]

Directly replace the canonical IV when we dissolve the containing region. That ensures that it won't get removed before the region gets removed, which would result in an invalid region.

This removes the current ordering constraint between convertToConcreteRecipes and dissolving regions.

[llvmbot]

@llvm/pr-subscribers-vectorizers

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

Directly replace the canonical IV when we dissolve the containing region. That ensures that it won't get removed before the region gets removed, which would result in an invalid region.

This removes the current ordering constraint between convertToConcreteRecipes and dissolving regions.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Vectorize/VPlan.cpp (+10)
• (modified) llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp (+3-7)

diff --git a/llvm/lib/Transforms/Vectorize/VPlan.cpp b/llvm/lib/Transforms/Vectorize/VPlan.cpp
index 165b57c87beb1..2580031c9186b 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlan.cpp
@@ -883,6 +883,16 @@ void VPRegionBlock::print(raw_ostream &O, const Twine &Indent,
 
 void VPRegionBlock::dissolveToCFGLoop() {
   auto *Header = cast<VPBasicBlock>(getEntry());
+  if (auto *CanIV = dyn_cast<VPCanonicalIVPHIRecipe>(&Header->front())) {
+    assert(this == getPlan()->getVectorLoopRegion() &&
+           "Canonical IV must be in the entry of the top-level loop region");
+    auto *ScalarR = Builder(CanIV).createScalarPhi(
+        {CanIV->getStartValue(), CanIV->getBackedgeValue()},
+        CanIV->getDebugLoc(), "index");
+    CanIV->replaceAllUsesWith(ScalarR);
+    CanIV->eraseFromParent();
+  }
+
   VPBlockBase *Preheader = getSinglePredecessor();
   auto *ExitingLatch = cast<VPBasicBlock>(getExiting());
   VPBlockBase *Middle = getSingleSuccessor();
diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
index 5b42a9056b69e..5c6d7b0c5b529 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
@@ -2601,14 +2601,10 @@ void VPlanTransforms::convertToConcreteRecipes(VPlan &Plan,
   for (VPBasicBlock *VPBB : VPBlockUtils::blocksOnly<VPBasicBlock>(
            vp_depth_first_deep(Plan.getEntry()))) {
     for (VPRecipeBase &R : make_early_inc_range(*VPBB)) {
-      if (isa<VPCanonicalIVPHIRecipe, VPEVLBasedIVPHIRecipe>(&R)) {
-        auto *PhiR = cast<VPHeaderPHIRecipe>(&R);
-        StringRef Name =
-            isa<VPCanonicalIVPHIRecipe>(PhiR) ? "index" : "evl.based.iv";
-        VPBuilder Builder(PhiR);
-        auto *ScalarR = Builder.createScalarPhi(
+      if (auto *PhiR = dyn_cast<VPEVLBasedIVPHIRecipe>(&R)) {
+        auto *ScalarR = Builder(PhiR).createScalarPhi(
             {PhiR->getStartValue(), PhiR->getBackedgeValue()},
-            PhiR->getDebugLoc(), Name);
+            PhiR->getDebugLoc(), "evl.based.iv");
         PhiR->replaceAllUsesWith(ScalarR);
         ToRemove.push_back(PhiR);
         continue;

[lukel97]

Makes sense, LGTM

[lukel97]

Just an idea, alternatively it could also be replaced in VPlanTransforms::dissolveLoopRegions where you could get it directly from getCanonicalIV()?

[fhahn]

Yep that would also do the trick, but doing it here has the advantage it cannot be missed by potential other users

[david-arm]

Looks like there are build failures:

C:\ws\src\llvm\lib\Transforms\Vectorize\VPlan.cpp(889): error C3861: 'Builder': identifier not found
C:\ws\src\llvm\lib\Transforms\Vectorize\VPlan.cpp(892): error C3536: 'ScalarR': cannot be used before it is initialized
C:\ws\src\llvm\lib\Transforms\Vectorize\VPlan.cpp(892): error C2664: 'void llvm::VPValue::replaceAllUsesWith(llvm::VPValue *)': cannot convert argument 1 from 'int' to 'llvm::VPValue *'
C:\ws\src\llvm\lib\Transforms\Vectorize\VPlan.cpp(892): note: Conversion from integral type to pointer type requires reinterpret_cast, C-style cast or function-style cast
C:\ws\src\llvm\lib\Transforms\Vectorize\VPlanValue.h(156): note: see declaration of 'llvm::VPValue::replaceAllUsesWith'

[fhahn]

Looks like there are build failures:

C:\ws\src\llvm\lib\Transforms\Vectorize\VPlan.cpp(889): error C3861: 'Builder': identifier not found
C:\ws\src\llvm\lib\Transforms\Vectorize\VPlan.cpp(892): error C3536: 'ScalarR': cannot be used before it is initialized
C:\ws\src\llvm\lib\Transforms\Vectorize\VPlan.cpp(892): error C2664: 'void llvm::VPValue::replaceAllUsesWith(llvm::VPValue *)': cannot convert argument 1 from 'int' to 'llvm::VPValue *'
C:\ws\src\llvm\lib\Transforms\Vectorize\VPlan.cpp(892): note: Conversion from integral type to pointer type requires reinterpret_cast, C-style cast or function-style cast
C:\ws\src\llvm\lib\Transforms\Vectorize\VPlanValue.h(156): note: see declaration of 'llvm::VPValue::replaceAllUsesWith'

Thanks, windows build failure should be fixed

[fhahn]

Yep that would also do the trick, but doing it here has the advantage it cannot be missed by potential other users

[ayalz]

Post-commit follow-up comment: this raises the thought of coupling CanonicalIVPHI with its loop Region - also to be created together, i.e., within a createFromCFGLoop() complementing dissolveToCFGLoop(). A loop region could hold its CanonicalIVPHI rather than place it inside its header block, abstracting away CanonicalIVIncrement along with its cyclic dependence w/o risk of dce, guaranteeing CanonicalIVPHI's uniqueness.