[InstCombine] Use dyn_cast instead of cast in visitPHINode

[liaolucy]

Summary

Fix crash https://github.com/llvm/llvm-project/issues/57329

[fhahn]

Thanks for tracking this down! Could you please add an instcombine test case for the issue?

[liaolucy]

In D132542#3745150, @fhahn wrote:

Thanks for tracking this down! Could you please add an instcombine test case for the issue?

Any suggestions for test cases? Is it as simple as possible or the complete ir of the issue?

[fhahn]

In D132542#3745223, @liaolucy wrote:

In D132542#3745150, @fhahn wrote:

Thanks for tracking this down! Could you please add an instcombine test case for the issue?

Any suggestions for test cases? Is it as simple as possible or the complete ir of the issue?

As simple as possible to reproduce the issue by just running opt -passes=instcombine. You can use -mllvm -print-on-crash -mllvm -print-module-scope to get the module before the crash and then use the techniques from https://llvm.org/docs/HowToSubmitABug.html to reduce it to the minimal crashing IR.

[hiraditya]

My understanding is that a basic block with a PHI node should have first instruction as PHI

[craig.topper]

In D132542#3747321, @hiraditya wrote:

My understanding is that a basic block with a PHI node should have first instruction as PHI

That understanding matches the IR verifier

void Verifier::visitPHINode(PHINode &PN) {                                       
  // Ensure that the PHI nodes are all grouped together at the top of the block. 
  // This can be tested by checking whether the instruction before this is       
  // either nonexistent (because this is begin()) or is a PHI node.  If not,     
  // then there is some other instruction before a PHI.                          
  Check(&PN == &PN.getParent()->front() ||                                       
            isa<PHINode>(--BasicBlock::iterator(&PN)),                           
        "PHI nodes not grouped at top of basic block!", &PN, PN.getParent());

[liaolucy]

Yes, PHI node should have first instruction as PHI.

Crash:

L3.preheader:                                     ; preds = %L3.preheader.preheader, %L2.loopexit
  %11 = zext i16 %10 to i32 -------------------------------------------------------------------------first
  %i.130 = phi i32 [ 0, %L2.loopexit ], [ %rem20.zext, %L3.preheader.preheader ]
  %tobool5.not = icmp eq i32 %i.130, 0
  br label %L3

No crash：

L3.preheader:                                     ; preds = %L3.preheader.preheader, %L2.loopexit
  %i.130 = phi i32 [ 0, %L2.loopexit ], [ %rem20.zext, %L3.preheader.preheader ]
  %11 = zext i16 %10 to i32 --------------------------------------------------------------------------change to second
  %tobool5.not = icmp eq i32 %i.130, 0
  br label %L3

It maybe the crash caused by this patch: https://reviews.llvm.org/D129710

[liaolucy]

In D132542#3745241, @fhahn wrote:

In D132542#3745223, @liaolucy wrote:

In D132542#3745150, @fhahn wrote:

Thanks for tracking this down! Could you please add an instcombine test case for the issue?

Any suggestions for test cases? Is it as simple as possible or the complete ir of the issue?

As simple as possible to reproduce the issue by just running opt -passes=instcombine. You can use -mllvm -print-on-crash -mllvm -print-module-scope to get the module before the crash and then use the techniques from https://llvm.org/docs/HowToSubmitABug.html to reduce it to the minimal crashing IR.

Sorry I can't add test cases for the time being, because there will be a failure.

/update_test_checks.py --opt=opt pr57329.ll 
PHI nodes not grouped at top of basic block!
  %i.129 = phi i32 [ 0, %L2 ], [ %0, %L1 ]

I'm looking for a better way to fix bug.

[lattner]

It has been a very long time since I've worked in this code, but I think the bug is up-stream, whatever is causing the phi nodes to not be adjacent

[liaolucy]

I've verified that D132571 solves the problem.

Without D132571:

L3.preheader:                                     ; preds = %L3.preheader.preheader, %L2.loopexit
  %11 = zext i16 %10 to i32
  %i.129 = phi i32 [ 0, %L2.loopexit ], [ %rem19.zext, %L3.preheader.preheader ]
  %tobool5.not = icmp eq i32 %i.129, 0
  br label %L3

With D132571, we get:

L1.backedge.loopexit44:                           ; preds = %L2.loopexit
  %1 = zext i16 %8 to i32
  br label %L1.backedge
....
L3.preheader:                                     ; preds = %L3.preheader.preheader, %L2.loopexit
  %i.131 = phi i32 [ 0, %L2.loopexit ], [ %rem22.zext, %L3.preheader.preheader ]
  %tobool7.not = icmp eq i32 %i.131, 0
  br label %L3

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp

if (InValNo != NumIncomingVals) {
       }
     }
   }
 
   // If there are multiple PHIs, sort their operands so that they all list
   // the blocks in the same order. This will help identical PHIs be eliminated
   // by other passes. Other passes shouldn't depend on this for correctness
   // however.
-  PHINode *FirstPN = cast<PHINode>(PN.getParent()->begin());
-  if (&PN != FirstPN)
+  PHINode *FirstPN = dyn_cast<PHINode>(PN.getParent()->begin());
+  if (FirstPN && &PN != FirstPN)
     for (unsigned I = 0, E = FirstPN->getNumIncomingValues(); I != E; ++I) {
       BasicBlock *BBA = PN.getIncomingBlock(I);
       BasicBlock *BBB = FirstPN->getIncomingBlock(I);
       if (BBA != BBB) {
         Value *VA = PN.getIncomingValue(I);
         unsigned J = PN.getBasicBlockIndex(BBB);
         Value *VB = PN.getIncomingValue(J);
         PN.setIncomingBlock(I, BBB);