[llvm-commits] [llvm] r125319 - in /llvm/trunk: lib/Analysis/ValueTracking.cpp test/Transforms/JumpThreading/degenerate-phi.ll

[Duncan Sands]

Hi,

>>>>> I think it would be better to have jump-threading zap
>>>>> any zero operand phis when it makes them, rather than changing all analyses to
>>>>> support them.
>>>>
>>>> It doesn't create them, it calls BasicBlock::removePredecessor.
>>>
>>> clearly it must clean them up somewhere (since phi nodes with no operands are
>>> invalid), so can't it just clean them up earlier?
>>
>> It looks to me like the problem is that removePredecessor() respects
>> the 'DontDeleteUselessPHIs' parameter even when the only predecessor
>> is removed, and that JumpThreading always passes 'true' for that (even
>> though AFAICT it doesn't have a good reason to).
>>
>> The best fix is probably to ignore that parameter if it's removing the
>> last predecessor; IMHO in this case it should just replace all PHIs
>> with undefs and then kill them, regardless of that parameter.
>
> I agree, that does seem like the right fix.

it seems dangerous to erase phi's when explicitly told not to - it creates the
danger of the caller using freed memory and so on.  Instead, how about having
removePredecessor always do a RAUW of degenerate phis with undef, but not
actually erase them if DontDeleteUselessPHIs was passed.

Ciao, Duncan.