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

[Duncan Sands]

Hi Mark,

> --- a/lib/Transforms/Utils/Local.cpp
> +++ b/lib/Transforms/Utils/Local.cpp
> @@ -525,6 +525,22 @@ void llvm::MergeBasicBlockIntoOnlyPred(BasicBlock *DestBB, Pass *P) {
>    PredBB->eraseFromParent();
>  }
>
> +/// CanMergeValues - Return true if we can choose one of these values to use
> +/// in place of the other. Note that we will always choose the non-undef
> +/// value to keep.
> +static bool CanMergeValues(Value *First, Value *Second) {
> +  return First == Second || isa<UndefValue>(First) || isa<UndefValue>(Second);
> +}
> +
> +/// ChooseMergedValue - Choose between First and Second, choosing the non-undef
> +/// value if there is one.
> +static Value *ChooseMergedValue(Value *First, Value *Second)  {
> +  assert(CanMergeValues(First, Second) && "Attempting to merge two values "
> +         "that are not equal and where neither is UndefValue.");

I think this message is too detailed.  You shouldn't describe algorithms in
assertion messages.  How about something like this: "Values not mergeable!"
or "Attempting to merge two values that are not mergeable!".

> +
> +  return !isa<UndefValue>(First) ? First : Second;
> +}
> +
>  /// CanPropagatePredecessorsForPHIs - Return true if we can fold BB, an
>  /// almost-empty BB ending in an unconditional branch to Succ, into succ.
>  ///
> @@ -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?

> +            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.

> +        }
>        } else {
> -        // Add an incoming value for each of the new incoming values.
> -        for (unsigned i = 0, e = BBPreds.size(); i != e; ++i)
> -          PN->addIncoming(OldVal, BBPreds[i]);
> +        for (unsigned i = 0, e = BBPreds.size(); i != e; ++i) {
> +          // If the successor phi already has an incoming edge for
> +          // this predecessor and the incoming value from BB or this
> +          // predecessor is undef, choose the non-undef value to use
> +          // if there is one.
> +          BasicBlock *PredBB = BBPreds[i];
> +          int PredBBIdx = PN->getBasicBlockIndex(PredBB);
> +          if (PredBBIdx >= 0) {

Same comment about whether the test is needed.

> +            Value *Existing = PN->getIncomingValue(PredBBIdx);
> +            OldVal = ChooseMergedValue(Existing, OldVal);

Is rewriting OldVal here a good idea?  It seems harmless, but now it's not loop
invariant any more.  Maybe just use a new local variable here?

> +            PN->setIncomingValue(PredBBIdx, OldVal);
> +          }
> +
> +          PN->addIncoming(OldVal, PredBB);

Same comment about this line.

Ciao, Duncan.