[PATCH] D17279: [Sink] Don't move calls to readonly functions across stores

[Daniel Berlin via llvm-commits]

Doesn't AA have an existing API that gives you exactly the info you want
here (Does this call ref the memory touched the things in the store list)


On Mon, Feb 15, 2016 at 4:47 PM, Nicolai Hähnle <llvm-commits at lists.llvm.org
> wrote:

> nhaehnle created this revision.
> nhaehnle added reviewers: hfinkel, majnemer, tstellarAMD, sunfish.
> nhaehnle added a subscriber: llvm-commits.
>
> This is conservative because it doesn't consider the argmemonly attribute.
> Perhaps somebody who knows this part of the code better can improve this
> in a separate patch. For now, it fixes a real miscompilation I encountered
> in the AMDGPU backend.
>
> http://reviews.llvm.org/D17279
>
> Files:
>   lib/Transforms/Scalar/Sink.cpp
>   test/Transforms/Sink/call.ll
>
> Index: test/Transforms/Sink/call.ll
> ===================================================================
> --- /dev/null
> +++ test/Transforms/Sink/call.ll
> @@ -0,0 +1,68 @@
> +; RUN: opt < %s -basicaa -sink -S | FileCheck %s
> +
> +declare i32 @f_load_global() nounwind readonly
> +declare void @f_store_global(i32) nounwind
> +declare i32 @f_readnone(i32) nounwind readnone
> +
> + at A = external global i32
> +
> +; Sink readonly call if no stores are in the way.
> +;
> +; CHECK-LABEL: @test1(
> +; CHECK: true:
> +; CHECK-NEXT: %l = call i32 @f_load_global
> +; CHECK-NEXT: ret i32 %l
> +define i32 @test1(i1 %z) {
> +  %l = call i32 @f_load_global()
> +  br i1 %z, label %true, label %false
> +true:
> +  ret i32 %l
> +false:
> +  ret i32 0
> +}
> +
> +; But don't sink if there is a store ...
> +;
> +; CHECK-LABEL: @test2(
> +; CHECK: call i32 @f_load_global
> +; CHECK-NEXT: store i32
> +define i32 @test2(i1 %z) {
> +  %l = call i32 @f_load_global()
> +  store i32 0, i32* @A
> +  br i1 %z, label %true, label %false
> +true:
> +  ret i32 %l
> +false:
> +  ret i32 0
> +}
> +
> +; ... or a non-readonly call
> +;
> +; CHECK-LABEL: @test3(
> +; CHECK: call i32 @f_load_global
> +; CHECK-NEXT: call void @f_store_global
> +define i32 @test3(i1 %z) {
> +  %l = call i32 @f_load_global()
> +  call void @f_store_global(i32 0)
> +  br i1 %z, label %true, label %false
> +true:
> +  ret i32 %l
> +false:
> +  ret i32 0
> +}
> +
> +; readnone calls are sunk across stores.
> +;
> +; CHECK-LABEL: @test4(
> +; CHECK: true:
> +; CHECK-NEXT: %l = call i32 @f_readnone(
> +; CHECK-NEXT: ret i32 %l
> +define i32 @test4(i1 %z) {
> +  %l = call i32 @f_readnone(i32 0)
> +  store i32 0, i32* @A
> +  br i1 %z, label %true, label %false
> +true:
> +  ret i32 %l
> +false:
> +  ret i32 0
> +}
> Index: lib/Transforms/Scalar/Sink.cpp
> ===================================================================
> --- lib/Transforms/Scalar/Sink.cpp
> +++ lib/Transforms/Scalar/Sink.cpp
> @@ -173,11 +173,14 @@
>        Inst->mayThrow())
>      return false;
>
> -  // Convergent operations cannot be made control-dependent on additional
> -  // values.
>    if (auto CS = CallSite(Inst)) {
> +    // Convergent operations cannot be made control-dependent on
> additional
> +    // values.
>      if (CS.hasFnAttr(Attribute::Convergent))
>        return false;
> +
> +    if (Inst->mayReadFromMemory() && !Stores.empty())
> +      return false;
>    }
>
>    return true;
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160215/6335877b/attachment.html>