[PATCH] D33179: [Sink] Fix predicate in legality check

[via llvm-commits]

This definitely doesn’t look right (looking at this because of a regression on our side).

Surely this should include anything that reads memory, not just loads? Also surely stores, too, because of WAW hazards? What about side effecting instructions?

—escha

> On Jun 9, 2017, at 12:31 PM, Phabricator via Phabricator via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> This revision was automatically updated to reflect the committed changes.
> Closed by commit rL305102: [Sink] Fix predicate in legality check (authored by kfischer).
> 
> Changed prior to commit:
>  https://reviews.llvm.org/D33179?vs=98943&id=102066#toc
> 
> Repository:
>  rL LLVM
> 
> https://reviews.llvm.org/D33179
> 
> Files:
>  llvm/trunk/lib/Transforms/Scalar/Sink.cpp
>  llvm/trunk/test/Transforms/Sink/badloadsink.ll
> 
> 
> Index: llvm/trunk/lib/Transforms/Scalar/Sink.cpp
> ===================================================================
> --- llvm/trunk/lib/Transforms/Scalar/Sink.cpp
> +++ llvm/trunk/lib/Transforms/Scalar/Sink.cpp
> @@ -114,7 +114,7 @@
>   if (SuccToSinkTo->getUniquePredecessor() != Inst->getParent()) {
>     // We cannot sink a load across a critical edge - there may be stores in
>     // other code paths.
> -    if (!isSafeToSpeculativelyExecute(Inst))
> +    if (isa<LoadInst>(Inst))
>       return false;
> 
>     // We don't want to sink across a critical edge if we don't dominate the
> Index: llvm/trunk/test/Transforms/Sink/badloadsink.ll
> ===================================================================
> --- llvm/trunk/test/Transforms/Sink/badloadsink.ll
> +++ llvm/trunk/test/Transforms/Sink/badloadsink.ll
> @@ -0,0 +1,18 @@
> +; RUN: opt < %s -basicaa -sink -S | FileCheck %s
> +declare void @foo(i64 *)
> +define i64 @sinkload(i1 %cmp) {
> +; CHECK-LABEL: @sinkload
> +top:
> +    %a = alloca i64
> +; CHECK: call void @foo(i64* %a)
> +; CHECK-NEXT: %x = load i64, i64* %a
> +    call void @foo(i64* %a)
> +    %x = load i64, i64* %a
> +    br i1 %cmp, label %A, label %B
> +A:
> +    store i64 0, i64 *%a
> +    br label %B
> +B:
> +; CHECK-NOT: load i64, i64 *%a
> +    ret i64 %x
> +}
> 
> 
> <D33179.102066.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits