[PATCH] Fixup PHI nodes in LowerSwitch
Philip Reames
listmail at philipreames.com
Wed Jul 9 15:29:44 PDT 2014
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
More information about the llvm-commits
mailing list