[VPlan] Fix header phi VPInstruction verification. NFC #151472

lukel97 · 2025-07-31T08:28:04Z

Noticed this when checking the invariant that all phis in the header block must be header phis. I think there's a missing set of parentheses here, since otherwise it only cast when RecipeI isn't a VPInstruction.

Noticed this when checking the invariant that all phis in the header block must be header phis. I think there's a missing set of parentheses here, since otherwise it only cast<VPInstruction> when RecipeI isn't a VPInstruction.

llvmbot · 2025-07-31T08:28:34Z

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-vectorizers

Author: Luke Lau (lukel97)

Changes

Noticed this when checking the invariant that all phis in the header block must be header phis. I think there's a missing set of parentheses here, since otherwise it only cast<VPInstruction> when RecipeI isn't a VPInstruction.

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

1 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp (+2-2)

diff --git a/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp b/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
index 57d01cbefbe26..b0de97ac5618d 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
@@ -80,8 +80,8 @@ bool VPlanVerifier::verifyPhiRecipes(const VPBasicBlock *VPBB) {
       NumActiveLaneMaskPhiRecipes++;
 
     if (IsHeaderVPBB && !isa<VPHeaderPHIRecipe, VPWidenPHIRecipe>(*RecipeI) &&
-        !isa<VPInstruction>(*RecipeI) &&
-        cast<VPInstruction>(RecipeI)->getOpcode() == Instruction::PHI) {
+        !(isa<VPInstruction>(*RecipeI) &&
+          cast<VPInstruction>(RecipeI)->getOpcode() == Instruction::PHI)) {
       errs() << "Found non-header PHI recipe in header VPBB";
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
       errs() << ": ";

Mel-Chen

LGTM, but I'd like to know if there's a way to find a test case to verify this?

fhahn

Can you add a test with a non-header-phi in header block to llvm/unittests/Transforms/Vectorize/VPlanVerifierTest.cpp?

I guess at the moment this would just be VPPredInstPHI?

fhahn · 2025-07-31T08:39:01Z

llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp

    if (isa<VPActiveLaneMaskPHIRecipe>(RecipeI))
      NumActiveLaneMaskPhiRecipes++;

    if (IsHeaderVPBB && !isa<VPHeaderPHIRecipe, VPWidenPHIRecipe>(*RecipeI) &&


Suggested change

if (IsHeaderVPBB && !isa<VPHeaderPHIRecipe, VPWidenPHIRecipe>(*RecipeI) &&

if (IsHeaderVPBB && !isa<VPHeaderPHIRecipe, VPWidenPHIRecipe, VPPhi>(*RecipeI) &&

This can just use VPPhi I think.

david-arm · 2025-07-31T09:00:22Z

llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp

    if (IsHeaderVPBB && !isa<VPHeaderPHIRecipe, VPWidenPHIRecipe>(*RecipeI) &&
-        !isa<VPInstruction>(*RecipeI) &&
-        cast<VPInstruction>(RecipeI)->getOpcode() == Instruction::PHI) {
+        !(isa<VPInstruction>(*RecipeI) &&


Looking at the definition of isPhi it seems like we're missing tests where we have a VPPredInstPHIRecipe in the header VPBB, since that seems to be the only one (besides VPInstructions) where isPhi would return true and isa<VPHeaderPHIRecipe, VPWidenPHIRecipe>(*RecipeI) would return false.

But looking at VPPredInstPHIRecipe it seems unlikely to be in the header, right? So perhaps there is no way to write a test case that proves this was broken.

I think a VPIRPhi, i.e a VPIRInstruction with a PHI opcode should also be caught by this, I've added a test case for it in c170996

lukel97 · 2025-07-31T09:07:42Z

Can you add a test with a non-header-phi in header block to llvm/unittests/Transforms/Vectorize/VPlanVerifierTest.cpp?

I guess at the moment this would just be VPPredInstPHI?

I added a test case in c170996 before I saw your comment, I think VPIRPhis are considered a non-header-phi too.

artagnon

Thanks for adding the test! I will let one of the other reviewers sign off on this.

artagnon · 2025-07-31T09:09:08Z

llvm/unittests/Transforms/Vectorize/VPlanVerifierTest.cpp

+      new VPInstruction(VPInstruction::BranchOnCond, {CanIV});
+
+  VPBasicBlock *VPBB1 = Plan.getEntry();
+  VPBasicBlock *VPBB2 = Plan.createVPBasicBlock("");


Suggested change

VPBasicBlock *VPBB2 = Plan.createVPBasicBlock("");

VPBasicBlock *VPBB2 = Plan.createVPBasicBlock("entry");

VPBB2 is the vector header block, so I named it "header"

llvm/unittests/Transforms/Vectorize/VPlanVerifierTest.cpp

fhahn

LGTM, thanks

llvm-ci · 2025-07-31T15:32:42Z

LLVM Buildbot has detected a new failure on builder llvm-clang-x86_64-win-fast running on as-builder-3 while building llvm at step 7 "test-build-unified-tree-check-llvm-unit".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/2/builds/30229

Here is the relevant piece of the build log for the reference

Step 7 (test-build-unified-tree-check-llvm-unit) failure: test (failure)
******************** TEST 'LLVM-Unit :: Transforms/Vectorize/./VectorizeTests.exe/50/52' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\build\unittests\Transforms\Vectorize\.\VectorizeTests.exe-LLVM-Unit-6452-50-52.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=52 GTEST_SHARD_INDEX=50 C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\build\unittests\Transforms\Vectorize\.\VectorizeTests.exe
--

Script:
--
C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\build\unittests\Transforms\Vectorize\.\VectorizeTests.exe --gtest_filter=VPVerifierTest.NonHeaderPHIInHeader
--
C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\llvm-project\llvm\unittests\Transforms\Vectorize\VPlanVerifierTest.cpp(317): error: Expected equality of these values:
  "Found non-header PHI recipe in header VPBB: IR   <badref> = phi i32 \n"
  ::testing::internal::GetCapturedStderr().c_str()
    Which is: "Found non-header PHI recipe in header VPBB"


C:\buildbot\as-builder-3\llvm-clang-x86_64-win-fast\llvm-project\llvm\unittests\Transforms\Vectorize\VPlanVerifierTest.cpp:317
Expected equality of these values:
  "Found non-header PHI recipe in header VPBB: IR   <badref> = phi i32 \n"
  ::testing::internal::GetCapturedStderr().c_str()
    Which is: "Found non-header PHI recipe in header VPBB"



********************

lukel97 · 2025-07-31T16:05:41Z

Looks like I need to change the expected output in the test whenever dumping isn't defined, working on a fix now.

artagnon · 2025-07-31T16:08:03Z

Maybe just do a prefix-matching?

lukel97 · 2025-07-31T16:18:31Z

Maybe just do a prefix-matching?

I wish I could, but doesn't look like there's a GTEST macro for prefixes unfortunately. I've just copied what the other tests have done and pushed a fix in 3e579d9

…152336) This PR fixes the issue that caused an ub in PR #151472. The issue was a shl call taking a negative shift amount (posDiff). The result was never used, but tablegen would perform the calculation anyway. The fix was to replace the shl call with just multiplications with constants. Original PR description: This patch improves the helper classes in the SpacemiT-X60 vector scheduling model and will be used in follow-up PRs: There are now two functions to map LMUL to values: * ConstValueUntilLMULThenDoubleBase: returns BaseValue for LMUL values before startLMUL, Value for startLMUL, then doubles Value for each subsequent LMUL. Useful for cases where fractional LMULs have constant cycles, and integer LMULs double as they increase. * GetLMULValue: takes an ordered list of LMUL cycles and LMUL and returns the corresponding cycle. Useful for cases we can't easily cover with ConstValueUntilLMULThenDoubleBase. This PR also adds some useful simplified versions of ConstValueUntilLMULThenDoubleBase, e.g.: ConstValueUntilLMULThenDouble (when BaseValue == Value), or ConstOneUntilMF4ThenDouble (when cycles start to double after MF2)

…lvm#152336) This PR fixes the issue that caused an ub in PR llvm#151472. The issue was a shl call taking a negative shift amount (posDiff). The result was never used, but tablegen would perform the calculation anyway. The fix was to replace the shl call with just multiplications with constants. Original PR description: This patch improves the helper classes in the SpacemiT-X60 vector scheduling model and will be used in follow-up PRs: There are now two functions to map LMUL to values: * ConstValueUntilLMULThenDoubleBase: returns BaseValue for LMUL values before startLMUL, Value for startLMUL, then doubles Value for each subsequent LMUL. Useful for cases where fractional LMULs have constant cycles, and integer LMULs double as they increase. * GetLMULValue: takes an ordered list of LMUL cycles and LMUL and returns the corresponding cycle. Useful for cases we can't easily cover with ConstValueUntilLMULThenDoubleBase. This PR also adds some useful simplified versions of ConstValueUntilLMULThenDoubleBase, e.g.: ConstValueUntilLMULThenDouble (when BaseValue == Value), or ConstOneUntilMF4ThenDouble (when cycles start to double after MF2)

[VPlan] Fix header phi VPInstruction verification. NFC

1ac3071

Noticed this when checking the invariant that all phis in the header block must be header phis. I think there's a missing set of parentheses here, since otherwise it only cast<VPInstruction> when RecipeI isn't a VPInstruction.

lukel97 requested review from Mel-Chen, artagnon, david-arm and fhahn July 31, 2025 08:28

llvmbot added vectorizers llvm:transforms labels Jul 31, 2025

Mel-Chen approved these changes Jul 31, 2025

View reviewed changes

fhahn reviewed Jul 31, 2025

View reviewed changes

david-arm reviewed Jul 31, 2025

View reviewed changes

lukel97 added 2 commits July 31, 2025 17:03

Add unit test

c170996

Use isa<VPPhi>

f11ad61

artagnon reviewed Jul 31, 2025

View reviewed changes

Address comments

77f1e7f

fhahn approved these changes Jul 31, 2025

View reviewed changes

lukel97 merged commit 08c5944 into llvm:main Jul 31, 2025
9 checks passed

mikhailramalho mentioned this pull request Aug 6, 2025

[Reland][RISCV] Refactor X60 scheduling model helper classes. NFC. #152336

Merged

	if (IsHeaderVPBB && !isa<VPHeaderPHIRecipe, VPWidenPHIRecipe>(*RecipeI) &&
	if (IsHeaderVPBB && !isa<VPHeaderPHIRecipe, VPWidenPHIRecipe, VPPhi>(*RecipeI) &&

	VPBasicBlock *VPBB2 = Plan.createVPBasicBlock("");
	VPBasicBlock *VPBB2 = Plan.createVPBasicBlock("entry");

[VPlan] Fix header phi VPInstruction verification. NFC #151472

[VPlan] Fix header phi VPInstruction verification. NFC #151472

Uh oh!

Conversation

lukel97 commented Jul 31, 2025

Uh oh!

llvmbot commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mel-Chen left a comment

Choose a reason for hiding this comment

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

fhahn Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

david-arm Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

david-arm Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

lukel97 Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

lukel97 commented Jul 31, 2025

Uh oh!

artagnon left a comment

Choose a reason for hiding this comment

Uh oh!

artagnon Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

lukel97 Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvm-ci commented Jul 31, 2025

Uh oh!

lukel97 commented Jul 31, 2025

Uh oh!

artagnon commented Jul 31, 2025

Uh oh!

lukel97 commented Jul 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

llvmbot commented Jul 31, 2025 •

edited

Loading