Skip to content

Conversation

@offsake
Copy link
Contributor

@offsake offsake commented Mar 20, 2025

Adding the checks for LoopForBB not being nullptr before try to dereference it.

Adding the checks for LoopForBB not being nullptr before try to dereference it.
@offsake offsake marked this pull request as ready for review March 20, 2025 23:55
@llvmbot
Copy link
Member

llvmbot commented Mar 20, 2025

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-vectorizers

Author: None (offsake)

Changes

Adding the checks for LoopForBB not being nullptr before try to dereference it.


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

1 Files Affected:

  • (modified) llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp (+3-3)
diff --git a/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp b/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
index 4b8a2420b3037..467c83395b812 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
@@ -379,7 +379,7 @@ void PlainCFGBuilder::buildPlainCFG(
     VPRegionBlock *Region = VPBB->getParent();
     Loop *LoopForBB = LI->getLoopFor(BB);
     // Set VPBB predecessors in the same order as they are in the incoming BB.
-    if (!isHeaderBB(BB, LoopForBB)) {
+    if (LoopForBB && !isHeaderBB(BB, LoopForBB)) {
       setVPBBPredsFromBB(VPBB, BB);
     } else if (Region) {
       // BB is a loop header and there's a corresponding region, set the
@@ -390,7 +390,7 @@ void PlainCFGBuilder::buildPlainCFG(
     // Create VPInstructions for BB.
     createVPInstructionsForVPBB(VPBB, BB);
 
-    if (BB == TheLoop->getLoopLatch()) {
+    if (LoopForBB && BB == TheLoop->getLoopLatch()) {
       VPBasicBlock *HeaderVPBB = getOrCreateVPBB(LoopForBB->getHeader());
       VPBlockUtils::connectBlocks(VPBB, HeaderVPBB);
       continue;
@@ -423,7 +423,7 @@ void PlainCFGBuilder::buildPlainCFG(
     BasicBlock *IRSucc1 = BI->getSuccessor(1);
     VPBasicBlock *Successor0 = getOrCreateVPBB(IRSucc0);
     VPBasicBlock *Successor1 = getOrCreateVPBB(IRSucc1);
-    if (BB == LoopForBB->getLoopLatch()) {
+    if (LoopForBB && BB == LoopForBB->getLoopLatch()) {
       // For a latch we need to set the successor of the region rather than that
       // of VPBB and it should be set to the exit, i.e., non-header successor,
       // except for the top region, which is handled elsewhere.

@offsake
Copy link
Contributor Author

offsake commented Mar 20, 2025

Hello @fhahn,

Could you please help me with reviewing the change?

I think the issue is introduced with 1395cd0:
[VPlan] Support multi-exit loops in HCFG builder.

If the change looks good to you, could you please merge it in as I don't have permission for doing that.

Thanks!

@offsake
Copy link
Contributor Author

offsake commented Mar 27, 2025

Hello @fhahn,
gently ping.

Copy link
Contributor

@fhahn fhahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you adda test case showing the issue?

@offsake
Copy link
Contributor Author

offsake commented Mar 31, 2025

Hello @fhahn ,

LoopBlocksRPO RPO(TheLoop); code might guarantee that all visited blocks have an associated loop. It means that the check below is redundant, which I removed in the second commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants