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

[Mark Lacey]

Hi Duncan,

Thanks for taking a look at my patch.

On Jun 6, 2013, at 1:55 AM, Duncan Sands <duncan.sands at gmail.com> wrote:
> Hi Mark,
> 
>> +/// 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!".

Good point. I've updated it.

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

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.

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

In the attached update I went ahead and added a new locally scoped to add clarity and avoid problems if the code is extended further and for some reason the original value of OldVal is needed.

Thanks again - updated patch attached.

Mark
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Allow-blocks-to-be-merged-when-one-has-an-undef-inpu.patch
Type: application/octet-stream
Size: 16128 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130606/321d4488/attachment.obj>
-------------- next part --------------