[llvm] r242199 - GVN: tolerate an instruction being replaced without existing in the leaderboard

[Nick Lewycky]

Tim Northover wrote:
> Author: tnorthover
> Date: Tue Jul 14 16:03:18 2015
> New Revision: 242199
>
> URL: http://llvm.org/viewvc/llvm-project?rev=242199&view=rev
> Log:
> GVN: tolerate an instruction being replaced without existing in the leaderboard
>
> Sometimes an incidentally created instruction can duplicate a Value used
> elsewhere. It then often doesn't end up in the leader table. If it's later
> removed, we attempt to remove it from the leader table and segfault.

This sounds very suspicious to me. While I agree that this fix does not 
miscompile, it sounds like you've fixed a symptom and that the 
underlying problem is inserting an instruction mid-GVN and not updating 
the leader table. Could you explain why that isn't a bug?

> Instead we should just ignore the removal request, which won't cause any
> problems. The reverse situation, where the original instruction is replaced by
> the new one (which you might think could leave the leader table empty) cannot
> occur, because the incidental instruction will never be found in the first
> place.
>
> Added:
>      llvm/trunk/test/Transforms/GVN/pre-new-inst.ll
> Modified:
>      llvm/trunk/lib/Transforms/Scalar/GVN.cpp
>
> Modified: llvm/trunk/lib/Transforms/Scalar/GVN.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVN.cpp?rev=242199&r1=242198&r2=242199&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/GVN.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/GVN.cpp Tue Jul 14 16:03:18 2015
> @@ -656,11 +656,14 @@ namespace {
>         LeaderTableEntry* Prev = nullptr;
>         LeaderTableEntry* Curr =&LeaderTable[N];
>
> -      while (Curr->Val != I || Curr->BB != BB) {
> +      while (Curr&&  (Curr->Val != I || Curr->BB != BB)) {
>           Prev = Curr;
>           Curr = Curr->Next;
>         }
>
> +      if (!Curr)
> +        return;
> +
>         if (Prev) {
>           Prev->Next = Curr->Next;
>         } else {
>
> Added: llvm/trunk/test/Transforms/GVN/pre-new-inst.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVN/pre-new-inst.ll?rev=242199&view=auto
> ==============================================================================
> --- llvm/trunk/test/Transforms/GVN/pre-new-inst.ll (added)
> +++ llvm/trunk/test/Transforms/GVN/pre-new-inst.ll Tue Jul 14 16:03:18 2015
> @@ -0,0 +1,29 @@
> +; RUN: opt -basicaa -gvn -S %s | FileCheck %s
> +
> +%MyStruct = type { i32, i32 }
> +define i8 @foo(i64 %in, i8* %arr) {
> +  %addr = alloca %MyStruct
> +  %dead = trunc i64 %in to i32
> +  br i1 undef, label %next, label %tmp
> +
> +tmp:
> +  call void @bar()
> +  br label %next
> +
> +next:
> +  %addr64 = bitcast %MyStruct* %addr to i64*
> +  store i64 %in, i64* %addr64
> +  br label %final
> +
> +final:
> +  %addr32 = getelementptr %MyStruct, %MyStruct* %addr, i32 0, i32 0
> +  %idx32 = load i32, i32* %addr32
> +
> +; CHECK: %resptr = getelementptr i8, i8* %arr, i32 %dead
> +  %resptr = getelementptr i8, i8* %arr, i32 %idx32
> +  %res = load i8, i8* %resptr
> +
> +  ret i8 %res
> +}
> +
> +declare void @bar()
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>