[llvm] r192001 - UpdatePHINodes in BasicBlockUtils should not crash on duplicate predecessors

[Quentin Colombet]

Hi Hal,

Your commit triggers a question regarding LLVM IR.

I may have misread the language reference manual but I did not see any mention of this limitation, i.e., that if a phi reference several time the same predecessor it must have the same value.
I said that it is a limitation because code like this:
define i32 @test(i32 %arg) {
entry:
  %cond = icmp sle i32 %arg, 412 
  br i1 %cond, label %end, label %end
end:
  %res = phi i32 [ 0, %entry ], [ 1, %entry ]
  ret i32 %res
}

Are perfectly legal from SSA stand-point.
Although it is a weird way of doing a select, you may end up with code like this when optimizations kick in (maybe not in llvm) :).

The verifier module aborts on this input code.
Therefore:
1. The verifier confirms your point (setting the same value on both paths works).
2. Is it something we want to fix in the compiler or should we “fix" the documentation?

Cheers,
-Quentin

On Oct 4, 2013, at 4:41 PM, Hal Finkel <hfinkel at anl.gov> wrote:

> Author: hfinkel
> Date: Fri Oct  4 18:41:05 2013
> New Revision: 192001
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=192001&view=rev
> Log:
> UpdatePHINodes in BasicBlockUtils should not crash on duplicate predecessors
> 
> UpdatePHINodes has an optimization to reuse an existing PHI node, where it
> first deletes all of its entries and then replaces them. Unfortunately, in the
> case where we had duplicate predecessors (which are allowed so long as the
> associated PHI entries have the same value), the loop removing the existing PHI
> entries from the to-be-reused PHI would assert (if that PHI was not the one
> which had the duplicates).
> 
> Added:
>    llvm/trunk/test/Transforms/LoopSimplify/dup-preds.ll
> Modified:
>    llvm/trunk/lib/Transforms/Utils/BasicBlockUtils.cpp
> 
> Modified: llvm/trunk/lib/Transforms/Utils/BasicBlockUtils.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/BasicBlockUtils.cpp?rev=192001&r1=192000&r2=192001&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Utils/BasicBlockUtils.cpp (original)
> +++ llvm/trunk/lib/Transforms/Utils/BasicBlockUtils.cpp Fri Oct  4 18:41:05 2013
> @@ -400,8 +400,12 @@ static void UpdatePHINodes(BasicBlock *O
>       // If all incoming values for the new PHI would be the same, just don't
>       // make a new PHI.  Instead, just remove the incoming values from the old
>       // PHI.
> -      for (unsigned i = 0, e = Preds.size(); i != e; ++i)
> -        PN->removeIncomingValue(Preds[i], false);
> +      for (unsigned i = 0, e = Preds.size(); i != e; ++i) {
> +        // Explicitly check the BB index here to handle duplicates in Preds.
> +        int Idx = PN->getBasicBlockIndex(Preds[i]);
> +        if (Idx >= 0)
> +          PN->removeIncomingValue(Idx, false);
> +      }
>     } else {
>       // If the values coming into the block are not the same, we need a PHI.
>       // Create the new PHI node, insert it into NewBB at the end of the block
> 
> Added: llvm/trunk/test/Transforms/LoopSimplify/dup-preds.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/LoopSimplify/dup-preds.ll?rev=192001&view=auto
> ==============================================================================
> --- llvm/trunk/test/Transforms/LoopSimplify/dup-preds.ll (added)
> +++ llvm/trunk/test/Transforms/LoopSimplify/dup-preds.ll Fri Oct  4 18:41:05 2013
> @@ -0,0 +1,46 @@
> +; RUN: opt -loop-simplify -S %s | FileCheck %s
> +target datalayout = "E-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-f128:128:128-v128:128:128-n32:64"
> +target triple = "powerpc64-bgq-linux"
> +
> +define fastcc void @do_update_md([3 x float]* nocapture readonly %x) #0 {
> +entry:
> +  br i1 undef, label %if.end365, label %lor.lhs.false134
> +
> +lor.lhs.false134:                                 ; preds = %entry
> +  br i1 undef, label %lor.lhs.false138, label %if.end365
> +
> +lor.lhs.false138:                                 ; preds = %lor.lhs.false134
> +  br i1 undef, label %lor.lhs.false142, label %if.end365
> +
> +lor.lhs.false142:                                 ; preds = %lor.lhs.false138
> +  br i1 undef, label %for.body276.lr.ph, label %if.end365
> +
> +for.body276.lr.ph:                                ; preds = %lor.lhs.false142
> +  switch i16 undef, label %if.then288 [
> +    i16 4, label %for.body344
> +    i16 2, label %for.body344
> +  ]
> +
> +if.then288:                                       ; preds = %for.body276.lr.ph
> +  br label %for.body305
> +
> +for.body305:                                      ; preds = %for.body305, %if.then288
> +  br label %for.body305
> +
> +for.body344:                                      ; preds = %for.body344, %for.body276.lr.ph, %for.body276.lr.ph
> +  %indvar = phi i64 [ %indvar.next, %for.body344 ], [ 0, %for.body276.lr.ph ]
> +  %indvars.iv552 = phi i64 [ %indvars.iv.next553, %for.body344 ], [ 0, %for.body276.lr.ph ], [ 0, %for.body276.lr.ph ]
> +  %indvars.iv.next553 = add nuw nsw i64 %indvars.iv552, 1
> +  %indvar.next = add i64 %indvar, 1
> +  br label %for.body344
> +
> +; CHECK-LABEL: @do_update_md
> +; CHECK: %indvars.iv552 = phi i64 [ %indvars.iv.next553, %for.body344 ], [ 0, %for.body344.preheader ]
> +; CHECK: ret
> +
> +if.end365:                                        ; preds = %lor.lhs.false142, %lor.lhs.false138, %lor.lhs.false134, %entry
> +  ret void
> +}
> +
> +attributes #0 = { nounwind }
> +
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131004/29858ce4/attachment.html>