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

[via llvm-commits]

> On Jan 11, 2018, at 11:03 AM, Keno Fischer <keno at juliacomputing.com> wrote:
> 
> 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.

Is this necessary? I’m fairly sure this should already be covered here:

if (auto CS = CallSite(Inst)) {
    // Convergent operations cannot be made control-dependent on additional
    // values.
    if (CS.hasFnAttr(Attribute::Convergent))
      return false;

    for (Instruction *S : Stores)
      if (AA.getModRefInfo(S, CS) & MRI_Mod)
        return false;
  }

> 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 ;)

i’m not sure about the test because this only comes up (for us, at least) with out-of-tree intrinsics that access memory (ex: texture reads).

—escha
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180111/3eb9ad4c/attachment.html>