[llvm-commits] [llvm] r133512 - /llvm/trunk/lib/Transforms/Scalar/LoopDeletion.cpp

[Jay Foad]

On 21 June 2011 12:41, Frits van Bommel <fvbommel at gmail.com> wrote:
> On 21 June 2011 12:02, Jay Foad <jay.foad at gmail.com> wrote:
>> Log:
>> Don't use PN->replaceUsesOfWith() to change a PHINode's incoming blocks,
>> because it won't work after my phi operand changes, because the incoming
>> blocks will no longer be Uses.
>
>>   while (PHINode* P = dyn_cast<PHINode>(BI)) {
>> -    P->replaceUsesOfWith(exitingBlock, preheader);
>> +    int j = P->getBasicBlockIndex(exitingBlock);
>> +    assert(j >= 0 && "Can't find exiting block in exit block's phi node!");
>> +    P->setIncomingBlock(j, preheader);
>
> replaceUsesOfWith() can handle multiple uses of the same value, but
> this version only handles a single use. Are you sure there can only be
> a single incoming edge from each basic block here?

exitingBlock is an arbitrarily chosen member of exitingBlocks[]. The
code goes on to say:

    for (unsigned i = 1; i < exitingBlocks.size(); ++i)
      P->removeIncomingValue(exitingBlocks[i]);

which will only remove one incoming edge for each of the other members
of exitingBlocks[]. So yes, I'm fairly sure that either there's only a
single incoming edge from each exitingBlock, or the code was already
broken.


I did consider a completely different fix, which is to implement something like:

void PHINode::replaceUsesOfWith(Value *From, Value *To) {
  if (isa<BasicBlock>(From)) {
    assert(isa<BasicBlock>(To));
    for (block_iterator I = block_begin(), E = block_end(); I != E; ++I)
      if (*I == From)
        *I = To;
  } else {
    assert(!isa<BasicBlock>(To));
    User::replaceUsesOfWith(From, To);
  }
}

Would you prefer that?

Jay.