llvm dev - Sep 2008 - [LLVMdev] SwitchInstr::removeCase() doesn't remove PHINodes' predecessors

Hi,

I've been writing an optimization pass (described on the ML previously). 
Sometimes this pass removes some case entries from switch instructions, 
which cause an abort because removeCase() doesn't fix the PHINodes 
predecessors when needed.

e.g.:

define i32 @foo() nounwind {
ifthen:
 %call = call i32 @bar()

 switch i32 %call, label %myphi [
  i32 0, label %ifelse
  i32 1, label %ifelse
 ]

ifelse:
 br label %myphi

myphi:
 %ret.0 = phi i32 [ 0, %ifthen ], [ 1, %ifelse ]
 ret i32 %ret.0
}

Suppose I remove the default target (i.e. replace it with other target BB). 
I get the following error:

PHINode should have one entry for each predecessor of its parent basic 
block!
        %ret.0 = phi i32 [ 0, %ifthen ], [ 1, %ifelse ]         ; <i32> 
[#uses=1]
Broken module found, compilation aborted!

This is because myphi is not reachable from ifthen anymore. My question is: 
is this a bug (or missing feature) or do I need to take care of the PHI 
Nodes myself? If it is the later what's the best way to handle that? (from 
what I could see I would need to iterate over all the instructions in the 
target BB and fix PHI Nodes if the target block becomes unreachable).


Thanks,
Nuno

Nuno Lopes wrote:
> PHINode should have one entry for each predecessor of its parent basic 
> block!
>         %ret.0 = phi i32 [ 0, %ifthen ], [ 1, %ifelse ]         ;
<i32>
> [#uses=1]
> Broken module found, compilation aborted!
> 
> This is because myphi is not reachable from ifthen anymore. My question is:
> is this a bug (or missing feature) or do I need to take care of the PHI 
> Nodes myself?
You get to take care of it yourself.

  If it is the later what's the best way to handle that?
(from
> what I could see I would need to iterate over all the instructions in the 
> target BB and fix PHI Nodes if the target block becomes unreachable).
TargetBB->removePredecessor(SwitchBB). Note that you should call this 
before you call removeCase!

Nick

>> PHINode should have one entry for each predecessor of its parent basic
>> block!
>>         %ret.0 = phi i32 [ 0, %ifthen ], [ 1, %ifelse ]         ;
<i32>
>> [#uses=1]
>> Broken module found, compilation aborted!
>>
>> This is because myphi is not reachable from ifthen anymore. My question
>> is:
>> is this a bug (or missing feature) or do I need to take care of the PHI
>> Nodes myself?
>
> You get to take care of it yourself.
>
>  If it is the later what's the best way to handle that? (from
>> what I could see I would need to iterate over all the instructions in
the
>> target BB and fix PHI Nodes if the target block becomes unreachable).
>
> TargetBB->removePredecessor(SwitchBB). Note that you should call this
> before you call removeCase!
Ok, thank you!
It's working now. I was afraid that removePredecessor() wouldn't work if
the
switchBB had more than one jump to the TargetBB, but it works :)

Thanks,
Nuno