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

[Keno Fischer via llvm-commits]

I haven't looked at this code in a while, but I think you'll have to

1) Add an `else if (Inst->mayReadFromMemory()) { return false; }` after the
check for LoadInst in isSafeToMove as a
    correctness fix.
2) For memory reading instructions you care about add a similar check to
the one that's there for LoadInst to recover
    the optimization (if applicable)
3) Change the isa<LoadInst> to mayReadFromMemory in IsAcceptableTarget
4) Add a test for your regression ;)



On Thu, Jan 11, 2018 at 1:52 PM, via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> nevermind, i see side effects and stores are already covered by
> isSafeToMove, so it sounds like the straightforward fix here is to just
> replace isa<LoadInst> with mayReadFromMemory.
>
> —escha
>
>
> On Jan 11, 2018, at 10:46 AM, via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
> 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
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
> _______________________________________________
> 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/20180111/31accb76/attachment.html>