[llvm-commits] [llvm] r57025 - in /llvm/trunk: lib/Transforms/Utils/SimplifyCFG.cpp test/Transforms/SimplifyCFG/2008-10-03-SpeculativelyExecuteBeforePHI.ll

[Nick Lewycky]

Owen Anderson wrote:
> 
> On Oct 3, 2008, at 11:57 AM, Devang Patel wrote:
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> ======================================================================
>> --- llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp Fri Oct  3 
>> 13:57:37 2008
>> @@ -1051,7 +1051,7 @@
>>   BasicBlock::iterator InsertPos = BI;
>>   if (InsertPos != BIParent->begin())
>>     --InsertPos;
>> -  if (InsertPos == BrCond) {
>> +  if (InsertPos == BrCond && !isa<PHINode>(BrCond)) {
>>     SmallPtrSet<Instruction *, 4> BB1Insns;
>>     for(BasicBlock::iterator BB1I = BB1->begin(), BB1E = BB1->end();
>>         BB1I != BB1E; ++BB1I)
> 
> I'm not sure this is the right fix.  It handles this specific case, but 
> I'm not sure it works in general.  I think what you want is something 
> more like:
> 
> BasicBlock::iterator InsertPos = BI;
> if (&*InsertPos != BIParent->getFirstNonPHI())
>   --InsertPos;
> if (InsertPos == BrCond) ...
> 
> I think that will be more resilient to other situations.

Owen and I met over IRC about this. This code works because all PHINodes 
are guaranteed to be in a contiguous block at the top of the function 
anyways. If BrCond is a PHI node, InsertPos would be set to 'right 
before the branch', and work fine.

This fix also saves a call to getFirstNonPHI which is a linear scan over 
the linked-list of Instructions in a block. (Usually a very short list, 
but regardless.)

Nick