[PATCH] reset iterators when more than one dead code instruction is removed

[Nick Lewycky]

Gerolf Hoflehner wrote:
> Hi
>
> a few places invoke dead instruction removal, but assume only one
> instruction gets removed. This patch
> resets the iterators when dead instructions have been removed.
>
> Gerolf
>
>
> Index: lib/Transforms/Scalar/LoopInstSimplify.cpp
> ===================================================================
> --- lib/Transforms/Scalar/LoopInstSimplify.cpp (revision 206487)
> +++ lib/Transforms/Scalar/LoopInstSimplify.cpp (working copy)
> @@ -126,7 +126,15 @@
> ++NumSimplified;
> }
> }
> - LocalChanged |= RecursivelyDeleteTriviallyDeadInstructions(I, TLI);
> + bool res = RecursivelyDeleteTriviallyDeadInstructions(I, TLI);
> + if (res) {
> + // RecursivelyDeleteTriviallyDeadInstruction can remove
> + // more than one instruction, so simply incrementing the
> + // iterator does not work. When instructions get deleted
> + // re-iterate instead.
> + BI = BB->begin(); BE = BB->end();
> + LocalChanged |= res;

I'm confused by this, because RecursivelyDeleteTriviallyDeadInstruction 
deletes upwards -- it has to due to SSA form. But maybe your testcase 
will explain it, maybe RDTDI has logic for PHIs or something ...

> + }
>
>
> if (IsSubloopHeader && !isa<PHINode>(I))
> break;
> Index: lib/Transforms/Utils/SimplifyInstructions.cpp
> ===================================================================
> --- lib/Transforms/Utils/SimplifyInstructions.cpp (revision 206487)
> +++ lib/Transforms/Utils/SimplifyInstructions.cpp (working copy)
> @@ -75,7 +75,15 @@
> ++NumSimplified;
> Changed = true;
> }
> - Changed |= RecursivelyDeleteTriviallyDeadInstructions(I, TLI);
> + bool res = RecursivelyDeleteTriviallyDeadInstructions(I, TLI);
> + if (res) {
> + // RecursivelyDeleteTriviallyDeadInstruction can remove
> + // more than one instruction, so simply incrementing the
> + // iterator does not work. When instructions get deleted
> + // re-iterate instead.
> + BI = BB->begin(); BE = BB->end();
> + Changed |= res;
> + }
> }
>
>
> // Place the list of instructions to simplify on the next loop iteration
> Index: test/Transforms/InstSimplify/dead-code-removal.ll
> ===================================================================
> --- test/Transforms/InstSimplify/dead-code-removal.ll (revision 0)
> +++ test/Transforms/InstSimplify/dead-code-removal.ll (working copy)
> @@ -0,0 +1,15 @@
> +; RUN: opts -instsimplify -S < %s | FileCheck %s

... and there isn't a program named "opts". Your test is broken.

But the testcase is correct otherwise, the problem is indeed due to phi 
nodes. Assuming this doesn't introduce bad performance problems -- 
O(n^2) compile-time stuff -- then this is fine with the fix to the testcase.

Nick

> +
> +define void @foo() nounwind {
> + br i1 undef, label %1, label %4
> +
> +; <label>:1 ; preds = %1, %0
> +; CHECK-NOT: phi
> +; CHECK-NOT: sub
> + %2 = phi i32 [ %3, %1 ], [ undef, %0 ]
> + %3 = sub i32 0, undef
> + br label %1
> +
> +; <label>:4 ; preds = %0
> + ret void
> +}
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits