[llvm-dev] Possible bug in adjusting PHINode from removePredecessor?

[Hariharan Sandanagobalane via llvm-dev]

Hi David,
I agree that my change only tries to maintain SSA sanity, and doesn't
deal with the impact of removing an unreachable call. Over here, i am
only interested in ensuring that the rest of the phases get clean IR
to operate on.

Do you have any suggestions on an alternate way to fix this issue?

Thanks
Hari

On Wed, Aug 12, 2015 at 4:50 PM, David Majnemer
<david.majnemer at gmail.com> wrote:
> I suspect such a change will only paper over your actual problem.
>
> On Wed, Aug 12, 2015 at 2:39 PM, Hariharan Sandanagobalane via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
>>
>> The following change fixes this issue in trunk. Does this fix look good?
>>
>> Thanks
>> Hari
>>
>> $ svn diff PruneEH.cpp
>> Index: PruneEH.cpp
>> ===================================================================
>> --- PruneEH.cpp (revision 243617)
>> +++ PruneEH.cpp (working copy)
>> @@ -268,7 +268,7 @@
>>    std::vector<BasicBlock*> Succs(succ_begin(BB), succ_end(BB));
>>
>>    for (unsigned i = 0, e = Succs.size(); i != e; ++i)
>> -    Succs[i]->removePredecessor(BB);
>> +    Succs[i]->removePredecessor(BB, true);
>>
>>    BB->eraseFromParent();
>>  }
>>
>> On Mon, Aug 10, 2015 at 1:24 PM, Hariharan Sandanagobalane
>> <hariharan.gcc at gmail.com> wrote:
>> > Hi,
>> > Simple description of the problem below. I have code coming into
>> > pruneEH as follows
>> > fn a {
>> > entry:
>> > call fn b
>> > ...
>> >
>> > for_cond:
>> > %i = phi [1, entry] [%x, for_body]
>> > cmp $i with someval
>> > cond-br for_body or for_exit
>> >
>> > for_body:
>> > ...
>> > $x = $i + 1
>> > branch for_cond
>> >
>> > for_exit
>> > ...
>> > }
>> >
>> > PruneEH determines that the call to fn-b won't return. The code is
>> > modified thus.
>> >
>> > fn a {
>> > entry:
>> > call fn b
>> > unreachable insn /* Instructions after call to fn-b replaced with
>> > unreachable insn */
>> >
>> > for_cond: /* No path from entry block */
>> > %i = phi [%x, for_body]
>> > cmp $i with someval
>> > cond-br for_body or for_exit
>> >
>> > for_body:
>> > ...
>> > $x = $i + 1
>> > branch for_cond
>> >
>> > for_exit
>> > ...
>> > }
>> >
>> > which then becomes
>> >
>> > fn a {
>> > entry:
>> > call fn b
>> > unreachable insn /* Instructions after call to fn-b replaced with
>> > unreachable insn */
>> >
>> > for_cond: /* No path from entry block */
>> > cmp $x with someval
>> > cond-br for_body or for_exit
>> >
>> > for_body:
>> > ...
>> > $x = $x + 1
>> > branch for_cond
>> >
>> > for_exit
>> > ...
>> > }
>> >
>> > The instruction
>> > $x = $x + 1
>> > is obviously illegal in SSA form, which shows up as an infinite loop
>> > in value numbering.
>> >
>> > The source of the problem exists in BasicBlock::removePredecessor
>> > function in BasicBlock.cpp. The comment in that function describes
>> > this exact scenario
>> >
>> >  // If there are exactly two predecessors, then we want to nuke the PHI
>> > nodes
>> >  // altogether. However, we cannot do this, if this in this case:
>> >  //
>> >  // Loop:
>> >  //   %x = phi [X, Loop]
>> >  //   %x2 = add %x, 1        ;; This would become %x2 = add %x2, 1
>> >  //   br Loop                ;; %x2 does not dominate all uses
>> >  //
>> >  // This is because the PHI node input is actually taken from the
>> > predecessor
>> >  // basic block. The only case this can happen is with a self loop, so
>> > we
>> >  // check for this case explicitly now.
>> >
>> > but goes on to cause the same issue. There are 2 potential problems in
>> > this function.
>> >
>> > 1. The comment above describes a self-loop block. The same problem can
>> > occur in loops with more than 1 block, as our example shows. In
>> > general, this can happen when the predecessor being removed does not
>> > belong to the same loop level as the basic block containing the
>> > PhiNode.
>> > 2. The version which introduced this comment r2694 did implement the
>> > self-loop case okay. A subsequent change - revision 22664 - broke
>> > this.
>> >
>> > The revision 22664 dates back to 2005, so this issue probably has been
>> > around for 10 years. I am not sure why nobody else has seen a problem
>> > here.
>> >
>> > I saw this issue in a large testcase. I will try to get a small repro
>> > to illustrate the issue.
>> >
>> > Regards
>> > Hari
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>