[VPlan] Fix header phi VPInstruction verification. NFC

[lukel97]

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.

[llvmbot]

@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]

Suggested change

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

This can just use VPPhi I think.

[david-arm]

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.

[david-arm]

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.

[lukel97]

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]

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]

Suggested change

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

[lukel97]

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

[fhahn]

LGTM, thanks

[llvm-ci]

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]

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

[artagnon]

Maybe just do a prefix-matching?

[lukel97]

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