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

[via llvm-commits]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180111/5d8defe4/attachment-0001.html>