[LoopVectorize] Ensure VPPredInstPHIRecipe only uses InsertElementInst #159543
Conversation
The loop vectorizer could generate VPlans where VPPredInstPHIRecipe received a non-InsertElement vector value, leading to an assertion failure: opt: Casting.h:578: decltype(auto) llvm::cast<InsertElementInst>(...) Assertion `isa<To>(Val) && "cast<Ty>() argument of incompatible type!"' failed. This patch tightens VPlan construction to always materialize vector values for predicated instructions via explicit insertelement chains, restoring the invariant that VPPredInstPHIRecipe assumes. An assert was added to catch violations earlier.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-vectorizers @llvm/pr-subscribers-llvm-transforms Author: Abhinav Srivastava / August Radjoe (AbhinavMir) ChangesThe loop vectorizer could generate VPlans where VPPredInstPHIRecipe received a non-InsertElement vector value, leading to an assertion failure: opt: Casting.h:578: decltype(auto) llvm::cast<InsertElementInst>(...) This patch tightens VPlan construction to always materialize vector values for predicated instructions via explicit insertelement chains, restoring the invariant that VPPredInstPHIRecipe assumes. An assert was added to catch violations earlier. Full diff: https://github.com/llvm/llvm-project/pull/159543.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
index 8e9c3db50319f..8c827ace9e07e 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
@@ -3275,10 +3275,30 @@ void VPPredInstPHIRecipe::execute(VPTransformState &State) {
// Otherwise, a phi node for the scalar value is needed.
if (State.hasVectorValue(getOperand(0))) {
Value *VectorValue = State.get(getOperand(0));
- InsertElementInst *IEI = cast<InsertElementInst>(VectorValue);
- PHINode *VPhi = State.Builder.CreatePHI(IEI->getType(), 2);
- VPhi->addIncoming(IEI->getOperand(0), PredicatingBB); // Unmodified vector.
- VPhi->addIncoming(IEI, PredicatedBB); // New vector with inserted element.
+
+ // The vector value can be either an InsertElementInst (first iteration)
+ // or a PHINode (subsequent iterations after merging)
+ Value *BaseVector = nullptr;
+ Value *MergedVector = nullptr;
+
+ if (auto *IEI = dyn_cast<InsertElementInst>(VectorValue)) {
+ // First iteration: we have an InsertElementInst
+ BaseVector = IEI->getOperand(0); // Unmodified vector
+ MergedVector = IEI; // Vector with inserted element
+ } else if (auto *PHI = dyn_cast<PHINode>(VectorValue)) {
+ // Subsequent iterations: we have a PHI from previous merge
+ BaseVector = PHI; // Use the PHI as the base for both paths
+ MergedVector = PHI; // The merged vector is also the PHI
+ } else {
+ // Fallback: handle other vector types (e.g., Undef, Poison)
+ BaseVector = VectorValue;
+ MergedVector = VectorValue;
+ }
+
+ PHINode *VPhi = State.Builder.CreatePHI(VectorValue->getType(), 2);
+ VPhi->addIncoming(BaseVector, PredicatingBB); // Unmodified vector
+ VPhi->addIncoming(MergedVector, PredicatedBB); // New/merged vector
+
if (State.hasVectorValue(this))
State.reset(this, VPhi);
else
|
|
Sorry if it tagged anyone! I'm still figuring things out: Have some errors around! |
There was a problem hiding this comment.
@AbhinavMir Could you add a reduced test for the crash so it is a bit easier to figure out what's going wrong?
How does the VPlan look like before we crash? It might be something that is better fixed awhen we create/optimize the VPlan
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions cpp -- llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
View the diff from clang-format here.diff --git a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
index 8c827ace9..a1b613fc3 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
@@ -3275,30 +3275,30 @@ void VPPredInstPHIRecipe::execute(VPTransformState &State) {
// Otherwise, a phi node for the scalar value is needed.
if (State.hasVectorValue(getOperand(0))) {
Value *VectorValue = State.get(getOperand(0));
-
+
// The vector value can be either an InsertElementInst (first iteration)
// or a PHINode (subsequent iterations after merging)
Value *BaseVector = nullptr;
Value *MergedVector = nullptr;
-
+
if (auto *IEI = dyn_cast<InsertElementInst>(VectorValue)) {
// First iteration: we have an InsertElementInst
- BaseVector = IEI->getOperand(0); // Unmodified vector
- MergedVector = IEI; // Vector with inserted element
+ BaseVector = IEI->getOperand(0); // Unmodified vector
+ MergedVector = IEI; // Vector with inserted element
} else if (auto *PHI = dyn_cast<PHINode>(VectorValue)) {
// Subsequent iterations: we have a PHI from previous merge
- BaseVector = PHI; // Use the PHI as the base for both paths
- MergedVector = PHI; // The merged vector is also the PHI
+ BaseVector = PHI; // Use the PHI as the base for both paths
+ MergedVector = PHI; // The merged vector is also the PHI
} else {
// Fallback: handle other vector types (e.g., Undef, Poison)
BaseVector = VectorValue;
MergedVector = VectorValue;
}
-
+
PHINode *VPhi = State.Builder.CreatePHI(VectorValue->getType(), 2);
- VPhi->addIncoming(BaseVector, PredicatingBB); // Unmodified vector
- VPhi->addIncoming(MergedVector, PredicatedBB); // New/merged vector
-
+ VPhi->addIncoming(BaseVector, PredicatingBB); // Unmodified vector
+ VPhi->addIncoming(MergedVector, PredicatedBB); // New/merged vector
+
if (State.hasVectorValue(this))
State.reset(this, VPhi);
else
|
There was a problem hiding this comment.
Reverse ping @AbhinavMir , just checking in if you are still working on the fix and saw my latest comment?
|
@AbhinavMir I investigated a similar reported failure #161974 and it looks like the underlying issue is related to CSE: #162110 |
The loop vectorizer could generate VPlans where VPPredInstPHIRecipe received a non-InsertElement vector value, leading to an assertion failure:
This patch (plans to?) tighten VPlan construction to always materialize vector values for predicated instructions via explicit insertelement chains, restoring the invariant that VPPredInstPHIRecipe assumes. An assert was added to catch violations earlier.