[PATCH] Fixup PHI nodes in LowerSwitch

[Marcello Maggioni]

Thank you very much Philip for your very useful comments as always :)

Answers inline.

================
Comment at: lib/Transforms/Utils/LowerSwitch.cpp:138
@@ +137,3 @@
+ for (BasicBlock::iterator I = Succ->begin(),
+                           E = Succ->getFirstNonPHIOrDbgOrLifetime();
+      I != E; ++I) {
----------------
Philip Reames wrote:
> I know this was discussed in another review, but I'm pretty sure this is not necessary.  I have no objection to you using this construct - i.e. go ahead and submit - but it would be good to clarify on llvm-dev and revisit if need be.  If this construct is actually required, I have quite a bit of out of tree code to fix.  :)
Does anybody have a reference to this, so that we can be sure that getFirstNonPhi() can be used 100% safe? I also like more that solution (of course) if it is correct :)

================
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??");
----------------
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).

================
Comment at: test/Transforms/LowerSwitch/2014-06-23-PHIlowering.ll:2
@@ +1,3 @@
+; RUN: opt < %s -lowerswitch -S | FileCheck %s
+; CHECK: phi i32 [ 1, %NodeBlock ], [ 2, %1 ]
+; CHECK: phi i32 [ 0, %NewDefault ]
----------------
Philip Reames wrote:
> Please add a check-label or otherwise give context here.  Where are these phis expected? What should be around them?
> 
> Also, could you place them in the body of the test function?  That way, we can add more tests to this file.  You'll need to add a CHECK: test
Will do in the next revision!

http://reviews.llvm.org/D4298