<div dir="ltr">I haven't looked at this code in a while, but I think you'll have to<div><br></div><div>1) Add an `else if (Inst->mayReadFromMemory()) { return false; }` after the check for LoadInst in isSafeToMove as a</div><div> correctness fix.</div><div>2) For memory reading instructions you care about add a similar check to the one that's there for LoadInst to recover</div><div> the optimization (if applicable)</div><div>3) Change the isa<LoadInst> to mayReadFromMemory in IsAcceptableTarget</div><div>4) Add a test for your regression ;)</div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 11, 2018 at 1:52 PM, via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word;line-break:after-white-space">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 <span style="color:rgb(49,89,93);font-family:Menlo;font-size:11px;background-color:rgb(255,255,255)">mayReadFromMemory</span>.<br><div><br></div><div>—escha<div><div class="h5"><br><div><br><blockquote type="cite"><div>On Jan 11, 2018, at 10:46 AM, via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:</div><br class="m_1488364585131397928Apple-interchange-newline"><div><div>This definitely doesn’t look right (looking at this because of a regression on our side).<br><br>Surely this should include anything that reads memory, not just loads? Also surely stores, too, because of WAW hazards? What about side effecting instructions?<br><br>—escha<br><br><blockquote type="cite">On Jun 9, 2017, at 12:31 PM, Phabricator via Phabricator via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br><br>This revision was automatically updated to reflect the committed changes.<br>Closed by commit rL305102: [Sink] Fix predicate in legality check (authored by kfischer).<br><br>Changed prior to commit:<br> <a href="https://reviews.llvm.org/D33179?vs=98943&id=102066#toc" target="_blank">https://reviews.llvm.org/<wbr>D33179?vs=98943&id=102066#toc</a><br><br>Repository:<br> rL LLVM<br><br><a href="https://reviews.llvm.org/D33179" target="_blank">https://reviews.llvm.org/<wbr>D33179</a><br><br>Files:<br> llvm/trunk/lib/Transforms/<wbr>Scalar/Sink.cpp<br> llvm/trunk/test/Transforms/<wbr>Sink/badloadsink.ll<br><br><br>Index: llvm/trunk/lib/Transforms/<wbr>Scalar/Sink.cpp<br>==============================<wbr>==============================<wbr>=======<br>--- llvm/trunk/lib/Transforms/<wbr>Scalar/Sink.cpp<br>+++ llvm/trunk/lib/Transforms/<wbr>Scalar/Sink.cpp<br>@@ -114,7 +114,7 @@<br> if (SuccToSinkTo-><wbr>getUniquePredecessor() != Inst->getParent()) {<br> // We cannot sink a load across a critical edge - there may be stores in<br> // other code paths.<br>- if (!<wbr>isSafeToSpeculativelyExecute(<wbr>Inst))<br>+ if (isa<LoadInst>(Inst))<br> return false;<br><br> // We don't want to sink across a critical edge if we don't dominate the<br>Index: llvm/trunk/test/Transforms/<wbr>Sink/badloadsink.ll<br>==============================<wbr>==============================<wbr>=======<br>--- llvm/trunk/test/Transforms/<wbr>Sink/badloadsink.ll<br>+++ llvm/trunk/test/Transforms/<wbr>Sink/badloadsink.ll<br>@@ -0,0 +1,18 @@<br>+; RUN: opt < %s -basicaa -sink -S | FileCheck %s<br>+declare void @foo(i64 *)<br>+define i64 @sinkload(i1 %cmp) {<br>+; CHECK-LABEL: @sinkload<br>+top:<br>+ %a = alloca i64<br>+; CHECK: call void @foo(i64* %a)<br>+; CHECK-NEXT: %x = load i64, i64* %a<br>+ call void @foo(i64* %a)<br>+ %x = load i64, i64* %a<br>+ br i1 %cmp, label %A, label %B<br>+A:<br>+ store i64 0, i64 *%a<br>+ br label %B<br>+B:<br>+; CHECK-NOT: load i64, i64 *%a<br>+ ret i64 %x<br>+}<br><br><br><D33179.102066.patch>_________<wbr>______________________________<wbr>________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br></blockquote><br>______________________________<wbr>_________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br></div></div></blockquote></div><br></div></div></div></div><br>______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>