[PATCH] Allow blocks to be merged when one has an undef input to a phi.

[Duncan Sands]

Hi Mark,

>>> @@ -646,18 +664,43 @@ bool llvm::TryToSimplifyUncondBranchFromEmptyBlock(BasicBlock *BB) {
>>>        // in the PHI node are the entries from the old PHI.
>>>        if (isa<PHINode>(OldVal) && cast<PHINode>(OldVal)->getParent() == BB) {
>>>          PHINode *OldValPN = cast<PHINode>(OldVal);
>>> -        for (unsigned i = 0, e = OldValPN->getNumIncomingValues(); i != e; ++i)
>>> +        for (unsigned i = 0, e = OldValPN->getNumIncomingValues(); i != e;
>>> +             ++i) {
>>>            // Note that, since we are merging phi nodes and BB and Succ might
>>>            // have common predecessors, we could end up with a phi node with
>>>            // identical incoming branches. This will be cleaned up later (and
>>>            // will trigger asserts if we try to clean it up now, without also
>>> -          // simplifying the corresponding conditional branch).
>>> -          PN->addIncoming(OldValPN->getIncomingValue(i),
>>> -                          OldValPN->getIncomingBlock(i));
>>> +          // simplifying the corresponding conditional branch). Since we allow
>>> +          // merging when an incoming value is undef, we do need to pick
>>> +          // a non-undef value if one exists and use that so that we are
>>> +          // consistent on the incoming values.
>>> +          Value *PredVal = OldValPN->getIncomingValue(i);
>>> +          BasicBlock *PredBB = OldValPN->getIncomingBlock(i);
>>> +          int PredBBIdx = PN->getBasicBlockIndex(PredBB);
>>> +          if (PredBBIdx >= 0) {
>>
>> Can this test ever fail?
>
> Yes. Block BB can have predecessors that are not predecessors of Succ. When they have common predecessors, we need to ensure that the (already present) incoming value in the phi is updated to the chosen value.
>
>>> +            Value *Existing = PN->getIncomingValue(PredBBIdx);
>>> +            PredVal = ChooseMergedValue(Existing, PredVal);
>>> +            PN->setIncomingValue(PredBBIdx, PredVal);
>>> +          }
>>> +
>>> +          PN->addIncoming(PredVal, PredBB);
>>
>> This line seems pointless if the (PredBBIdx >= 0) test passed, so it could be
>> an "else" branch.
>
> All of the predecessors of BB must be added to the phi node since we rewrite all of the uses of BB (in terminators) to target Succ. Otherwise we would have a conditional branch or switch that branches to Succ on multiple edges (e.g. both sides of the conditional branch, or multiple cases of the switch), but not have as many incoming edges in the phi.

if (PredBBIdx >= 0) was true, then PN already has PredBB has a predecessor.
Indeed, you just assigned a value to it in the line
   PN->setIncomingValue(PredBBIdx, PredVal);
But then you fall through to the line
   PN->addIncoming(PredVal, PredBB);
and add a second copy of PredBB and PredVal to PN.  Isn't this wrong?

Ciao, Duncan.