[PATCH] Fixup PHI nodes in LowerSwitch

[Philip Reames]

p.s. We do enforce that PHIs are at the beginning of the basic block.  This is enforced by visitPHINode in the verifier.  The code in question is:
1511    // Ensure that the PHI nodes are all grouped together at the top of the block.                                                        
1512    // This can be tested by checking whether the instruction before this is                                                              
1513    // either nonexistent (because this is begin()) or is a PHI node.  If not,                                                            
1514    // then there is some other instruction before a PHI.                                                                                 
1515    Assert2(&PN == &PN.getParent()->front()||                                                                                            
1516            isa<PHINode>(--BasicBlock::iterator(&PN)),                                                                                    
1517            "PHI nodes not grouped at top of basic block!",                                                                               
1518            &PN, PN.getParent());

================
Comment at: lib/Transforms/Utils/LowerSwitch.cpp:145
@@ +144,3 @@
+
+   int BlockIdx = PN->getBasicBlockIndex(OrigBlock);
+   assert(BlockIdx != -1 && "Switch didn't go to this successor??");
----------------
Marcello Maggioni wrote:
> Philip Reames wrote:
> > Oh, I just noticed this.  A single basic block can reach a successor with different values.  (e.g. consider a switch which simply forwards the case value, before that's optimized away) 
> > 
> > The getBasicBlockIndex will return the first such edge, not all edges.  You probably want to write:
> > for each incoming edge: update if bb == predecessor
> > 
> > It would be good to add a test case for this if possible.  If you believe this is not possible, add a comment and assert that checks that.  (i.e. after the update, no remaining incoming edge from OrigBlock.)
> > 
> > 
> What do you mean exactly with this? You mean something like this? 
> 
> define i32 @foo(i32 %a) nounwind uwtable readnone ssp {
>   switch i32 %a, label %2 [
>     i32 1, label %3
>     i32 2, label %1
>     i32 3, label %3
>   ]
> 
> ; <label>:1                                       ; preds = %0
>   br label %3
> 
> ; <label>:2                                       ; preds = %0
>   br label %3
> 
> ; <label>:3                                       ; preds = %0, %3, %2, %1
>   %.0 = phi i32 [ 2, %2 ], [ 9, %0 ], [ 15, %1 ], [ 10, %0 ]
>   ret i32 %.0
> }
> 
> If this is the case the verifier on code like this gives this error:
> 
> PHI node has multiple entries for the same basic block with different incoming values!
>   %.0 = phi i32 [ 2, %2 ], [ 9, %0 ], [ 15, %1 ], [ 10, %0 ]
> label %0
> i32 10
> i32 9
> 
> You specified for different values in your comment, this seems to not be possible. With THE SAME value though I think it is (the verifier doesn't complain if i change the PHI to "%.0 = phi i32 [ 2, %2 ], [ 9, %0 ], [ 15, %1 ], [ 10, %0 ]").
>  I don't know if it can actually happen at the LowerSwitch phase, which is very low in the stack and after basically all IR optimizations, but considering that it is a possibility might be worth implementing it the way you specified (with the loop over all the incoming edges).
Your analysis is correct.  It looked like I misstated the case slightly.  Having the loop is still necessary for multiple incoming edges with the same value from the same basic block.

http://reviews.llvm.org/D4298